<div>Steve,</div>
<div> </div>
<div>Here's another patch for the DLL imports part of it, redone on the current versions. I made a change to it, however, inputting the _CMAKE_LIB_ definition for triggering the export verses import via a flag from the build instead of the CIndex.cpp file. This might avoid confusion in the future if additional files are added later to the DLL.<br>
</div>
<div>For the rest, I'll wait to hear from you.</div>
<div> </div>
<div>-John<br></div>
<div class="gmail_quote">On Tue, Oct 20, 2009 at 12:43 PM, steve naroff <span dir="ltr"><<a href="mailto:snaroff@apple.com">snaroff@apple.com</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div style="WORD-WRAP: break-word">Hi John,
<div><br></div>
<div>The patch didn't apply for me (it's based on yesterday's source). Could you update it?</div>
<div><br></div>
<div>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).</div>
<div><br></div>
<div>One spin on your change...</div>
<div><br></div>
<div>
<div>+// For temporary string returns.</div>
<div>+static char CIBuffer[256];</div>
<div>+</div>
<div><br></div>
<div>...is to have a static llvm::StringSet that contains copies of the strings. For example:</div>
<div><br></div>
<div>static llvm::StringSet<> LazilyComputedShortLivedNames;</div>
<div><br></div>
<div>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).</div>
<div><br></div>
<div>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.</div>
<div><br></div>
<div>So...it seem like the choices are:</div>
<div><br></div>
<div>(a) static StringSet<> to keep the API simple (i.e. no requirement on the client to size/deal with strings).</div>
<div>(b) per-index StringSet<> to keep the string API simple. Modify the API to pass CXIndex in more places.</div>
<div>(c) per-index StringSet<> to keep the string API simple. Add bookkeeping to navigate from a CXDecl->CXTranslationUnit->CXIndex.</div>
<div>(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.</div>
<div>(e) Go with your change (documenting the limitations).</div>
<div><br></div>
<div>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).</div>
<div><br></div>
<div>As a result, I want to get other opinions on this before we make a decision.</div>
<div><br></div>
<div>Thanks much,</div>
<div><br></div>
<div>snaroff</div>
<div><br></div>
<div>
<div>
<div></div>
<div class="h5">
<div>On Oct 20, 2009, at 3:15 PM, John Thompson wrote:</div><br></div></div>
<blockquote type="cite">
<div>
<div></div>
<div class="h5">
<div>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.)</div>
<div> </div>
<div>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.<br>
</div>
<div>-John<br></div>
<div class="gmail_quote">On Tue, Oct 20, 2009 at 8:36 AM, steve naroff <span dir="ltr"><<a href="mailto:snaroff@apple.com" target="_blank">snaroff@apple.com</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: #ccc 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div style="WORD-WRAP: break-word">Hi John,
<div><br></div>
<div>Comments below...</div>
<div><br>
<div>
<div>
<div>On Oct 20, 2009, at 11:03 AM, John Thompson wrote:</div><br>
<blockquote type="cite">
<div>Now that I have electricity again...</div>
<div> </div>
<div>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.</div>
<div> </div></blockquote>
<div><br></div></div>Nice catch. I guess other platforms are more forgiving...</div>
<div>
<div><br>
<blockquote type="cite">
<div>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.</div>
<div> </div></blockquote>
<div><br></div></div>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...</div>
<div>
<div><br>
<blockquote type="cite">
<div>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).</div>
<div> </div></blockquote>
<div><br></div></div>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...</div>
<div>
<div><br>
<blockquote type="cite">
<div>Note that I moved the getLocationFromCursor function out of the extern "C" {} because MSVC was complaining about it because of the object return type.</div>
<div> </div>
<div>Where does "basename" come from on the non-Windows platforms? It was unresolved, so I guessed at it, but which seems to work.</div>
<div> </div></blockquote>
<div><br></div></div>basename is a Unix/Linux function...not standard C or POSIX. Does Windows have something called _splitpath()?</div>
<div>
<div><br>
<blockquote type="cite">
<div>The c-index-api-test.m test passes on Windows with these changes.</div></blockquote>
<div><br></div></div>Great. Thanks for helping out on this!</div>
<div><br></div>
<div>snaroff</div>
<div><br>
<blockquote type="cite">
<div>
<div> </div>
<div>-John<br></div>
<div class="gmail_quote">On Mon, Oct 19, 2009 at 12:14 PM, John Thompson <span dir="ltr"><<a href="mailto:john.thompson.jtsoftware@gmail.com" target="_blank">john.thompson.jtsoftware@gmail.com</a>></span> wrote:<br>
<blockquote style="BORDER-LEFT: rgb(204,204,204) 1px solid; MARGIN: 0px 0px 0px 0.8ex; PADDING-LEFT: 1ex" class="gmail_quote">
<div>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.</div>
<div> </div>
<div>In the function Selector::getAsString function in IdentifierTable.cpp at line 330:</div>
<div> </div>
<div> return II->getName().str() + ":";<br><br clear="all">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.</div>
<div></div>
<div> </div>
<div>If someone more knowlegeable about it could take a look at it, I'd appreciate it.</div>
<div> </div>
<div>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.</div>
<div> </div>
<div>-John</div>
<div><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br><br></div></blockquote></div><br><br clear="all">
<div></div><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br><br></div><span><cindex.patch></span>_______________________________________________<br>
cfe-dev mailing list<br><a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
</blockquote></div><br></div></div></blockquote></div><br><br clear="all">
<div></div><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com" target="_blank">John.Thompson.JTSoftware@gmail.com</a><br><br></div></div><span><cindex_string.patch></span><span><cindex_imports.patch></span></blockquote>
</div><br></div></div></blockquote></div><br><br clear="all">
<div></div><br>-- <br>John Thompson<br><a href="mailto:John.Thompson.JTSoftware@gmail.com">John.Thompson.JTSoftware@gmail.com</a><br><br>