[PATCH] D39571: [clangd] DidChangeConfiguration Notification

Simon Marchi via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Feb 14 05:34:04 PST 2018


simark added a comment.

In https://reviews.llvm.org/D39571#1007291, @ilya-biryukov wrote:

> It looks like a bug in the preamble handling. (It does not check if macros were redefined).
>  You can workaround that by making sure the preamble ends before your code starts (preamble only captures preprocessor directives, so any C++ decl will end it):
>
>     Annotations SourceAnnotations(R"cpp(
>   int avoid_preamble;
>  
>   #ifndef MACRO
>   $before[[static void bob() {}]]
>   #else
>   $after[[static void bob() {}]]
>   #endif
>   /// ....
>   )cpp"
>


Ah ok, indeed this test now works when I add this.

The other test I am working on (at the bottom of the file) acts weird, even if I add `int avoid_preamble`.  The idea of the test is:

1. Set -DWITH_ERROR=1 in the compile commands database
2. Add the document, expect one error diagnostic
3. Unset WITH_ERROR in the compile commands database
4. Reparse the file, expect no error diagnostic

If I do it in this order, I get one diagnostic both times, when I don't expect one the second time the file is parsed.  But if I do it the other way (first parse with no errors, second parse with an error), it works fine.



================
Comment at: unittests/clangd/XRefsTests.cpp:260
 
+TEST(DidChangeConfiguration, DifferentDeclaration) {
+  Annotations SourceAnnotations(R"cpp(
----------------
ilya-biryukov wrote:
> I'd move it to `ClangdTests.cpp`, generic `ClangdServer` tests usually go there. It's fine to `#include "Annotations.h"` there, too, even though it hasn't been used before.
> 
Ok.  I put it there for rapid prototyping, but I also though it didn't really belong there.


================
Comment at: unittests/clangd/XRefsTests.cpp:271
+
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;
----------------
ilya-biryukov wrote:
> Specifying `/*UseRelPath=*/true` is not necessary for this test, default value should do.
Ok.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D39571





More information about the cfe-commits mailing list