Refactoring internal CXString APIs

Hello Argyrios and cfe-dev,

In order to fix the bug discussed in "createCXString reads one past
end byte", we need change CXString internal APIs: when creating a
CXString from a StringRef, we also need a TU, to find the StringPool.

I see this as a good opportunity to refactor internal CXString APIs.
What I want to do is to change current createCXString to be more
intuitive.

(1) Since these functions are in cxstring namespace, I want to drop
CXString from the name, *and* remove "using namespace cxstring"
everywhere. All calls would become cxstring::createWhatever().

(2) Introduce CXString createEmpty(); There are more than 70 places
in libclang where we create empty CXStrings and we 4 different ways to
do exactly the same thing:

createCXString("")
createCXString("", false)
createCXString((const char *) 0)
createCXString((const char *) NULL)

(3) libclang code is full of comments explaining the meaning of the
second parameter to createCXString:

createCXString(Cmd->CommandLine[Arg].c_str(), /*DupString=*/false)

If the API needs that, we should consider changing the API :slight_smile: And we
easily can:

CXString createRef(const char *String);
CXString createDup(const char *String);

CXString createRef(StringRef String);
CXString createDup(StringRef String);

CXString createRef(CXStringBuf *buf);

(4) After fixing that bug, create...(StringRef) will accept an
additional parameter -- TU.

(5) We can also change createDup(const char *String) to accept a TU,
so that we will use our string pool in all cases.

What do you think?

Dmitri

Hello Argyrios and cfe-dev,

In order to fix the bug discussed in "createCXString reads one past
end byte", we need change CXString internal APIs: when creating a
CXString from a StringRef, we also need a TU, to find the StringPool.

I see this as a good opportunity to refactor internal CXString APIs.
What I want to do is to change current createCXString to be more
intuitive.

(snip)

(3) libclang code is full of comments explaining the meaning of the
second parameter to createCXString:

createCXString(Cmd->CommandLine[Arg].c_str(), /*DupString=*/false)

If the API needs that, we should consider changing the API :slight_smile: And we
easily can:

This just proves that boolean parameters make the code harder to read.
Furthermore:

http://martinfowler.com/bliki/FlagArgument.html
http://silkandspinach.net/2004/07/15/avoid-boolean-parameters/
http://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong/

Csaba

Without having read those links an enum parameter could be used instead:

enum class DupString { no, yes };

createCXString(Cmd->CommandLine[Arg].c_str(), DupString::no);

Hello Argyrios and cfe-dev,

In order to fix the bug discussed in "createCXString reads one past
end byte", we need change CXString internal APIs: when creating a
CXString from a StringRef, we also need a TU, to find the StringPool.

I see this as a good opportunity to refactor internal CXString APIs.
What I want to do is to change current createCXString to be more
intuitive.

(1) Since these functions are in cxstring namespace, I want to drop
CXString from the name, *and* remove "using namespace cxstring"
everywhere. All calls would become cxstring::createWhatever().

Sounds good.

(2) Introduce CXString createEmpty(); There are more than 70 places
in libclang where we create empty CXStrings and we 4 different ways to
do exactly the same thing:

createCXString("")
createCXString("", false)
createCXString((const char *) 0)
createCXString((const char *) NULL)

Are you suggesting that we change places where we return a null string and return an empty one instead ?
I'm a bit uncomfortable with this; while I agree this situation is unfortunate and I don't like it, this is bound to break compatibility with clients where they get an empty string when they only checked for a null string.

For now I suggest also adding "createNull()" and retain current behavior.

(3) libclang code is full of comments explaining the meaning of the
second parameter to createCXString:

createCXString(Cmd->CommandLine[Arg].c_str(), /*DupString=*/false)

If the API needs that, we should consider changing the API :slight_smile: And we
easily can:

CXString createRef(const char *String);
CXString createDup(const char *String);

CXString createRef(StringRef String);
CXString createDup(StringRef String);

CXString createRef(CXStringBuf *buf);

(4) After fixing that bug, create...(StringRef) will accept an
additional parameter -- TU.

(5) We can also change createDup(const char *String) to accept a TU,
so that we will use our string pool in all cases.

What do you think?

These are good ideas!