sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang-tools-extra/clangd/SourceCode.cpp:261 + if (!Lexer::getRawToken(Loc, TheTok, SM, LangOpts)) { + if (TheTok.is(tok::greatergreater)) + return 1; ---------------- sammccall wrote: > `>>=` too? That's legal with variable templates. > > (If you're deliberately choosing different behavior that definitely deserves > a comment) nit: please don't just mark comments "done" if there's reason not to do them - a brief explanation in the review and/or the code is useful to keep track. For the record, it turns out that it seems C++ doesn't actually allow `>>=` to be ambiguous - the user has to put a space before = if it's not a shift. ================ Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:410 +// Test for functions toHalfOpenFileRange and getHalfOpenFileRange +// FIXME: Need better testing support to be able to check more than just Decls. +TEST(SourceCodeTests, HalfOpenFileRange) { ---------------- SureYeaah wrote: > sammccall wrote: > > this is a function on ranges, so only using decls isn't a limitation per se. > > > > Is there a type of range you're unable to test because you can't construct > > it as the source range of a decl? If so, please say which. If not I think > > we should just drop this comment. > e.g. Nested template instantiation (TemplateSpecializationTypeLoc) That's a type of decl, not a type of range. This function inputs and outputs a range, so the type of decl used isn't relevant to its correctness. Is there a particular type of range (e.g. certain macro expansion structure and pair of points) that you want to test but can't construct a decl to cover? ================ Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:421 + $c[[FOO(b, c)]]; + $d[[FOO(BAR(BAR(b)), d)]]; + } ---------------- SureYeaah wrote: > sammccall wrote: > > some tests where the expansion range is a macro arg? e.g. > > ``` > > #define ECHO(X) X > > ECHO($e[[ECHO(int) ECHO(e)]]) > > ``` > > > > (if I'm understanding right) > In this case, the FileRange would be > > ``` > ECHO(ECHO($e[[int) ECHO(e]])); > ``` > > So the code works correctly. But it's not how we want it to behave right? That looks OK to me. It's surprising, but I think defensible. The most likely source of complaints is I guess if we use selection tree to implement structured selection expansion. Anyway, whatever the behavior is, thanks for adding the test :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64562/new/ https://reviews.llvm.org/D64562 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits