sammccall added a comment.
I'm concerned about the scope of this patch - it does too many things and
touches too many files for me to feel comfortable that I understand it well
enough to review.
Is it possible to split it up? (You mention 6 distinct things in the
description).
Have tried to focus comments on what seems to be the core parts.
================
Comment at: clangd/CodeComplete.cpp:247
+// Calculates include path and insertion edit for a new header.
+class IncludeInsertion {
+public:
----------------
I can't parse this name as it relates to this class. This inserts includes, it
isn't itself an insertion.
Maybe `IncludeInserter`?
================
Comment at: clangd/CodeComplete.h:72
+ PathRef FileName, const tooling::CompileCommand &Command,
+ PrecompiledPreamble const *Preamble, const InclusionLocations
&IncLocations,
+ StringRef Contents, Position Pos, IntrusiveRefCntPtr<vfs::FileSystem> VFS,
----------------
IncLocations isn't an appropriate name for the variable - it's coupled to the
preamble
================
Comment at: clangd/Headers.h:1
//===--- Headers.h - Include headers -----------------------------*-
C++-*-===//
//
----------------
The interface to this header looks a bit complex and entangled after this
change.
- The construction/lifecycle of IncludePaths is confusing (comments below)
- here we provide all the information needed to compute the edits (style,
code...) but don't actually do so
it would be nice to achieve a simpler interface with the restricted set of
functionality, or expand the functionality...
================
Comment at: clangd/Headers.h:29
+// A header inclusion in the main file.
+struct InclusionLocation {
----------------
I had some trouble working out the role of this struct (e.g. that it might have
been a *proposed* inclusion).
Strictly it's not just the location, but also identifies the header being
included.
So this could maybe be
```
// An #include directive that we found in the main file.
struct Inclusion
```
but it does also seem a little strange that this is part of the public
interface at all (other than for testing)
================
Comment at: clangd/Headers.h:40
+
+using InclusionLocations = std::vector<InclusionLocation>;
+
----------------
This doesn't seem to add any semantics over the vector, so it just seems to
obscure a bit.
In fact it does have semantics: it's the stored list of inclusion locations for
the preamble. But those semantics don't belong in this layer. I'd suggest
moving it back to the preamble code, or avoiding the typedef altogether.
================
Comment at: clangd/Headers.h:55
-/// Determines the preferred way to #include a file, taking into account the
-/// search path. Usually this will prefer a shorter representation like
-/// 'Foo/Bar.h' over a longer one like 'Baz/include/Foo/Bar.h'.
-///
-/// \param File is an absolute file path.
-/// \param DeclaringHeader is the original header corresponding to \p
-/// InsertedHeader e.g. the header that declares a symbol.
-/// \param InsertedHeader The preferred header to be inserted. This could be
the
-/// same as DeclaringHeader but must be provided.
-// \return A quoted "path" or <path>. This returns an empty string if:
-/// - Either \p DeclaringHeader or \p InsertedHeader is already (directly)
-/// included in the file (including those included via different paths).
-/// - \p DeclaringHeader or \p InsertedHeader is the same as \p File.
-llvm::Expected<std::string>
-calculateIncludePath(PathRef File, llvm::StringRef Code,
- const HeaderFile &DeclaringHeader,
- const HeaderFile &InsertedHeader,
- const tooling::CompileCommand &CompileCommand,
- IntrusiveRefCntPtr<vfs::FileSystem> FS);
+/// Calculates #include paths for inserting new headers into a file, based on
+/// preprocessor and build configs.
----------------
the main value-add of this class over HeaderSearch is that it determines
whether to insert, so that should probably be mentioned.
================
Comment at: clangd/Headers.h:56
+/// Calculates #include paths for inserting new headers into a file, based on
+/// preprocessor and build configs.
+class IncludePaths {
----------------
can you describe the design a little bit here?
e.g. "We simply keep track of which files were already inserted, and delegate
to clang's HeaderSearch if a header is new"
================
Comment at: clangd/Headers.h:57
+/// preprocessor and build configs.
+class IncludePaths {
+public:
----------------
This name suggests a set or sequence of include path entries (`-I` flags).
Can we merge this with `IncludeInsertion` and call it `IncludeInserter` or
similar?
================
Comment at: clangd/Headers.h:60
+ /// This must be called before preprocessor is run (this registers a
+ /// PPCallback on \p PP).
+ /// \p SM is used in the PPCallbacks to filter source locations in main file.
----------------
hmm, if we require the InclusionLocations to be prebuilt for the preamble, why
can't we do the same for non-preamble includes? i.e. can't we split up the
"collect existing includes" and "calculate new includes" parts to simplify the
interface here? Among other things, testing would be a ton easier.
================
Comment at: clangd/Headers.h:63
+ /// \p IncLocations Known inclusions in the main file e.g. from preamble.
+ IncludePaths(StringRef File, StringRef BuildDirectory,
+ const SourceManager &SM, const InclusionLocations &IncLocations,
----------------
The contract around PP is really confusing: we need to be able to mutate it (to
add PP hooks), we need someone to use it in the middle, we need to use its
HeaderSearch structure later, and to add more confusion we have shared
ownership.
Maybe instead something like:
- this class takes an IncludeSearch& (sadly not const-correct) to use for
lookups
- it exposes a function like `std::unique_ptr<PPCallbacks> record()` which
returns a callback hook which writes into `this` includeptahs object.
Then usage looks like:
```
IncludePaths Paths(SM, PreambleLocs, PP.getHeaderSearch());
PP.addPPCallbacks(Paths.record());
// do something with PP
Paths.calculate(...);
```
and AFAICT, all the lifetime issues/interactions would be communicated by the
types.
================
Comment at: clangd/Headers.h:64
+ IncludePaths(StringRef File, StringRef BuildDirectory,
+ const SourceManager &SM, const InclusionLocations &IncLocations,
+ std::shared_ptr<Preprocessor> PP);
----------------
consider adding a `addExistingInclude(InclusionLocation)` instead of the
`InclusionLocations` parameter here. This gives you somewhere to document what
this does and what it's for (constructors are always crowded), and makes the
callsites a little more self-documenting.
It also lets you take InclusionLocations out of the public interface here,
which I think would be a win.
================
Comment at: clangd/Headers.h:83
+ std::string File;
+ std::string Code;
+ std::string BuildDirectory;
----------------
is this used?
================
Comment at: clangd/Headers.h:85
+ std::string BuildDirectory;
+ format::FormatStyle Style;
+
----------------
and this?
================
Comment at: clangd/Headers.h:95
+ std::shared_ptr<Preprocessor> PP;
+ std::unique_ptr<tooling::HeaderIncludes> Inserter;
+};
----------------
is this unused? this seems like it could be a natural part of this class...
================
Comment at: clangd/tool/ClangdMain.cpp:103
"0 means no limit."),
- llvm::cl::init(100));
+ llvm::cl::init(1000));
----------------
why this change?
================
Comment at: unittests/clangd/HeadersTests.cpp:48
+ IgnoringDiagConsumer IgnoreDiags;
+ auto CI = clang::createInvocationFromCommandLine(
+ Argv,
----------------
Is there any way we can avoid creating another copy of this code?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D46497
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
