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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Mostly nits around naming/doc: you've convinced me that the two headers that are paths or uris or <headers> at different times is the right thing, but we need to be really clear about it.



================
Comment at: clangd/ClangdServer.cpp:320
+                            StringRef PreferredHeader) {
+  auto Resolve = [File](StringRef Header) -> llvm::Expected<std::string> {
+    if (Header.empty() || Header.startswith("<") || Header.startswith("\""))
----------------
For readability, I'd suggest pulling this out into a real function, and returning something like
`struct HeaderFile { std::string File; bool Verbatim; }` which could be passed to the `Headers.h` functions.

The fold of "literal or uri" -> "literal or file" all typed as 'string' is a bit too much I think.
But maybe more explicit names and comments would moke this clearer.


================
Comment at: clangd/ClangdServer.h:239
+  /// Inserts a new #include into \p File, if it's not present in \p Code.
+  /// \p OriginalHeader The original header corresponding to this insertion.
+  /// This may be different from preferredHeader as a header file can have a
----------------
sammccall wrote:
> I really do want to know what this parameter does, but I don't understand it from this comment.
Discussed offline - this is the header file (either URI or <header>) that directly declares the symbol.
I'd suggest calling this `DeclaringHeader`.


================
Comment at: clangd/ClangdServer.h:240
+  /// \p OriginalHeader The original header corresponding to this insertion.
+  /// This may be different from preferredHeader as a header file can have a
+  /// different canonical include. This could be either a URI or a literal
----------------
the "this may be different" probably belongs after the doc of PreferredHeader (backreferences are easier to follow than forward ones)


================
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:
> "this has the same semantic as OriginalHeader" contradicts "[OriginalHeader] may be different from preferredHeader" :-)
T think `InsertedHeader` might be clearer about its role.


================
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:
> > "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".


================
Comment at: clangd/ClangdServer.h:244
+  /// \p PreferredHeader The preferred header to be inserted. The has the same
+  /// semantic as \p OriginalHeader. If empty, \p OriginalHeader is used to
+  /// calculate the #include path.
----------------
Drop the "empty" special case, the caller should always pass this in.


================
Comment at: clangd/ClangdServer.h:246
+  /// calculate the #include path.
   Expected<tooling::Replacements> insertInclude(PathRef File, StringRef Code,
+                                                StringRef OriginalHeader,
----------------
Worth pointing out at the end "Both OriginalHeader and InsertedHeader will be considered to determine whether an include needs to be added".


================
Comment at: clangd/Headers.cpp:72
+    return "";
+  if (OriginalHeader.empty() && PreferredHeader.empty())
     return "";
----------------
how could this happen?


================
Comment at: clangd/Headers.cpp:126
+  };
+  if (Included(OriginalHeader) || Included(PreferredHeader))
     return llvm::make_error<llvm::StringError>(
----------------
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?


================
Comment at: clangd/Headers.h:23
+/// Returns true if \p Include is literal include like "path" or <path>.
+bool isLiteralInclude(llvm::StringRef Include);
+
----------------
only used in headers.cpp, make static?


================
Comment at: clangd/Headers.h:29
 ///
+/// If non-empty, \p PreferredHeader is used to calculate the include path;
+/// otherwise, \p OriginalHeader is used.
----------------
again, this special case isn't needed as copying a stringref is cheap


================
Comment at: clangd/Headers.h:33
 /// \param File is an absolute file path.
-/// \param Header is an absolute file path.
-/// \return A quoted "path" or <path>. This returns an empty string if:
-///   - \p Header is already (directly) included in the file (including those
-///   included via different paths).
-///   - \p Header is the same as \p File.
+/// \param OriginalHeader is the original header corresponding to \p
+/// PreferredHeader. If non-empty, it is either an absolute path or a
----------------
when would originalheader be empty?


================
Comment at: clangd/Headers.h:37
+/// \param PreferredHeader The preferred header to be included, if non-empty.
+/// the semantic is the same as \p OriginalHeader.
+//  \return A quoted "path" or <path>. This returns an empty string if:
----------------
again - not quite true.


================
Comment at: clangd/Protocol.h:434
 
-  /// The header to be inserted. This could be either a URI ir a literal string
-  /// quoted with <> or "" that can be #included directly.
-  std::string header;
+  /// The original header corresponding to this insertion. This may be different
+  /// from preferredHeader as a header file can have a different canonical
----------------
(also update comments here)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43510