[PATCH] Record ranges skipped by the preprocessor and expose them with libclang.

[Argyrios Kyrtzidis]

Sorry for the long delay in getting back to you!

On Oct 31, 2013, at 9:36 AM, Erik Verbruggen <erikjv at me.com> wrote:

> 
> On 31 Oct 2013, at 0:57, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> 
>> Hi Erik, sorry for the delay!
>> 
>> On Oct 12, 2013, at 4:25 AM, Erik Verbruggen <erikjv at me.com> wrote:
>> 
>>> It has been some time since the last time I did a patch...
>>> 
>>> Record ranges skipped by the preprocessor and expose them with libclang.
>> 
>> Cool!
>> 
>>> 
>>> This requires the use of a detailed preprocessing record. Also bumbed the cindex minor version to reflect adding new functionality (and to be able to detect that during built-time).
>>> 
>>> The patch is against r192531, which has the same amount of failures for me on MacOS as trunk (two, both with OpenCL).
>>> 
>>> Feedback please! :)
>> 
>> Some nitpicks:
>> 
>> +/**
>> + * \brief Retrieve all ranges that were skipped by the preprocessor.
>> + */
>> +CINDEX_LINKAGE CXSkippedRanges *clang_getSkippedRanges(CXTranslationUnit tu);
>> 
>> Should we have a function to get all skipped ranges, and another to get the skipped ranges of a particular file ? IMO the latter is much more useful.
> 
> You're right of course. So for API design: add a const char *filename as second parameter?

I suggest a "CXFile file" parameter.

> 
>> +
>> +    /// \brief Retrieve all ranges that got skipped while preprocessing.
>> +    const std::vector<SourceRange> &getSkippedRanges() const {
>> +      return SkippedRanges;
>> +    }
>> 
>> Please use ArrayRef here.
>> 
>> Also we would need to serialize the skipped ranges to the preprocessing record of a PCH as well but this can be done later.
> 
> Do we need that? I mean, my use-case is to grey-out the skipped ranges/lines in an IDE. When a file is opened that's part of a PCH, it will probably need a reparse anyway... But I might be missing something here.

If you are just browsing the header we can get all the information from the PCH/precompiled preamble, we don't need to reparse.
Or do you mean something else ? I don't understand why we need to reparse anyway..

> 
>> +// RUN: env CINDEXTEST_SHOW_SKIPPED_RANGES=1 c-index-test -test-annotate-tokens=%s:1:1:16:1 %s | FileCheck %s
>> +// CHECK: Skipping: [5:2 - 6:7]
>> +// CHECK: Skipping: [8:2 - 12:7]
>> +// CHECK: Skipping: [14:2 - 20:7]
>> 
>> Should we just unconditionally (I mean without the CINDEXTEST_SHOW_SKIPPED_RANGES env variable) show the skipped ranges for the '-test-annotate-tokens' option (and update any test if it breaks) ?
> 
> Will do that.
> 
>> Also the test is not also testing if tokens are properly annotated/not-annotated, could you add some CHECK/CHECK-NOT lines for some of the tokens ?
> 
> Sorry, I don't understand your question. Can you give an example?

Never mind, it's not important.

> 
> I just noticed that my rebase removed the CINDEX_VERSION_MINOR bump. I'll add it back in, if that's okay?

Yes, certainly.

> 
> Thanks!
> -- Erik.
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131112/3b33235d/attachment.html>