[cfe-dev] Selector::getAsString problem in CIndex
steve naroff
snaroff at apple.com
Tue Oct 20 13:03:39 PDT 2009
Just after sending this, I realized there may be another AST-based
solution:
The ObjC AST's could implement the following:
const char *getSelectorAsCString(); // for multi-keyword selectors,
the result would be computed/cached.
This would make the semantics uniform with getNameAsCString(). Since
it looks like this API may be deprecated, maybe not a great idea.
Since this is only an issue when integrating a C-based API with C++,
it may be overkill to solve this issue at the AST level...
Nevertheless, I wanted to mention it for completeness...
snaroff
On Oct 20, 2009, at 3:43 PM, steve naroff wrote:
> Hi John,
>
> The patch didn't apply for me (it's based on yesterday's source).
> Could you update it?
>
> On the string lifetime issue...after consulting with Daniel, we'd
> prefer to avoid cluttering up the API to deal with lazily computed
> names (that aren't part of the AST per se).
>
> One spin on your change...
>
> +// For temporary string returns.
> +static char CIBuffer[256];
> +
>
> ...is to have a static llvm::StringSet that contains copies of the
> strings. For example:
>
> static llvm::StringSet<> LazilyComputedShortLivedNames;
>
> While this is more general than your change (no limit, can hold an
> arbitrary # of strings), it is static data (which is less than great).
>
> It would be nice if this data were cached per-Index. Unfortunately,
> this would require either (a) more bookkeeping or (b) more explicit
> 'CXIndex' arguments be passed around.
>
> So...it seem like the choices are:
>
> (a) static StringSet<> to keep the API simple (i.e. no requirement
> on the client to size/deal with strings).
> (b) per-index StringSet<> to keep the string API simple. Modify the
> API to pass CXIndex in more places.
> (c) per-index StringSet<> to keep the string API simple. Add
> bookkeeping to navigate from a CXDecl->CXTranslationUnit->CXIndex.
> (d) Avoid passing back C strings entirely (what I previously said
> we'd like to avoid). Complicates the API, but makes the contract
> very explicit.
> (e) Go with your change (documenting the limitations).
>
> At the moment, this is primarily a problem with ObjC names (i.e.
> Selectors), however this will be an issues for C++ as well I believe
> (for names that contain type info).
>
> As a result, I want to get other opinions on this before we make a
> decision.
>
> Thanks much,
>
> snaroff
>
> On Oct 20, 2009, at 3:15 PM, John Thompson wrote:
>
>> Here it is separated into two patches. cindex_imports has the DLL
>> stuff plus the basename fix. cindex_string has the string fixes
>> plus the function move. cindex_string depends on cindex_imports.
>> (The only overlapping part is the CIndex.cpp _CINDEX_CPP_
>> definition in cindex_imports.)
>>
>> Looking at _splitpath, it seems that my basename is more convenient
>> to use, since _splitpath breaks the path up into separate drive,
>> dir, fname, and ext strings, which need separate buffers. I'd also
>> be concerned as to whether it will work with unix-style '/'
>> directory separators, whereas mine works with both.
>> -John
>> On Tue, Oct 20, 2009 at 8:36 AM, steve naroff <snaroff at apple.com>
>> wrote:
>> Hi John,
>>
>> Comments below...
>>
>> On Oct 20, 2009, at 11:03 AM, John Thompson wrote:
>>
>>> Now that I have electricity again...
>>>
>>> Sorry, I was looking too deep, not noticing that the function
>>> returned a std::string. The problem was up higher in CIndex.cpp,
>>> in a couple of the functions that return const char *, with the
>>> same problem I described of the temporary going away before the
>>> string can be used.
>>>
>>
>> Nice catch. I guess other platforms are more forgiving...
>>
>>> I worked around it by using a static buffer, which is admittedly
>>> not so good. A cleaner way might be to have the user pass in a
>>> string buffer, since it seems this needs to be a C interface.
>>> Should I do that, or does the API need to remain unchanged? Let
>>> me know if there's a better way.
>>>
>>
>> I'd like to avoid cluttering the interface if possible. I'm not an
>> expert on std::String, so I'll have to look into this. I'll get
>> back to you...
>>
>>> The enclosed patch contains my hacked version, which also includes
>>> the DLL exporting glue (redone in the naming style I'm used to -
>>> apologies to Jeff Krall, who'd already done it).
>>>
>>
>> Can you separate this from the other stuff in the patch? I'd like
>> to keep this type of mechanical change separate from the trickier
>> stuff...
>>
>>> Note that I moved the getLocationFromCursor function out of the
>>> extern "C" {} because MSVC was complaining about it because of the
>>> object return type.
>>>
>>> Where does "basename" come from on the non-Windows platforms? It
>>> was unresolved, so I guessed at it, but which seems to work.
>>>
>>
>> basename is a Unix/Linux function...not standard C or POSIX. Does
>> Windows have something called _splitpath()?
>>
>>> The c-index-api-test.m test passes on Windows with these changes.
>>
>> Great. Thanks for helping out on this!
>>
>> snaroff
>>
>>>
>>> -John
>>> On Mon, Oct 19, 2009 at 12:14 PM, John Thompson <john.thompson.jtsoftware at gmail.com
>>> > wrote:
>>> I'm looking at a test failure of c-index-api-test.m on Windows,
>>> and I've run into a problem I don't know how to easily fix.
>>>
>>> In the function Selector::getAsString function in
>>> IdentifierTable.cpp at line 330:
>>>
>>> return II->getName().str() + ":";
>>>
>>> On Windows, at least, this isn't working as expected. I think
>>> it's because a tempory string created gets destructed before the
>>> return completes, causing the resulting string pointer to point to
>>> garbage, resulting in garbage for the identifier name in the index
>>> output.
>>>
>>> If someone more knowlegeable about it could take a look at it, I'd
>>> appreciate it.
>>>
>>> Note that if you need to run it on windows, you'll need some
>>> CIndex changes I'm working on, but haven't completed yet, so let
>>> me know if you need them.
>>>
>>> -John
>>>
>>> --
>>> John Thompson
>>> John.Thompson.JTSoftware at gmail.com
>>>
>>>
>>>
>>>
>>> --
>>> John Thompson
>>> John.Thompson.JTSoftware at gmail.com
>>>
>>> <cindex.patch>_______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>>
>>
>> --
>> John Thompson
>> John.Thompson.JTSoftware at gmail.com
>>
>> <cindex_string.patch><cindex_imports.patch>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20091020/89ac5dae/attachment.html>
More information about the cfe-dev
mailing list