[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