On 31 Oct 2013, at 0:57, Argyrios Kyrtzidis <[email protected]> wrote:
> Hi Erik, sorry for the delay! > > On Oct 12, 2013, at 4:25 AM, Erik Verbruggen <[email protected]> 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? > + > + /// \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. > +// 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? I just noticed that my rebase removed the CINDEX_VERSION_MINOR bump. I'll add it back in, if that's okay? Thanks! -- Erik. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
