[PATCH] D43510: [clangd] don't insert new includes if either original header or canonical header is already included.

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 20 09:02:33 PST 2018


ioeric added inline comments.


================
Comment at: clangd/ClangdServer.h:243
+  /// string quoted with <> or "" that can be #included directly.
+  /// \p PreferredHeader The preferred header to be inserted. The has the same
+  /// semantic as \p OriginalHeader. If empty, \p OriginalHeader is used to
----------------
sammccall wrote:
> sammccall wrote:
> > sammccall wrote:
> > > "this has the same semantic as OriginalHeader" contradicts "[OriginalHeader] may be different from preferredHeader" :-)
> > T think `InsertedHeader` might be clearer about its role.
> I'd suggest being explicit, since this is different to above: "if this is a URI, insertInclude will determine how to spell it. If this is a quoted string, it will be inserted verbatim".
Bad wording... sorry about that!


================
Comment at: clangd/Headers.cpp:126
+  };
+  if (Included(OriginalHeader) || Included(PreferredHeader))
     return llvm::make_error<llvm::StringError>(
----------------
sammccall wrote:
> oops, missed this in the previous commit - this isn't an error condition, but a "return empty string" condition.
> 
> Can we add a test for this?
There are tests for this in `HeadersTests.cpp`. It's just that in the tests, we treat both errors and empty insertion as no insertion.


================
Comment at: clangd/Headers.h:23
+/// Returns true if \p Include is literal include like "path" or <path>.
+bool isLiteralInclude(llvm::StringRef Include);
+
----------------
sammccall wrote:
> only used in headers.cpp, make static?
Oops, the same logic appeared in ClangdServer. I wanted to share this but forgot to...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43510





More information about the cfe-commits mailing list