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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits