kadircet added a comment.
thanks, mostly LG. some small changes.
================
Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:261
if (Symbol.empty())
+ F.Message = llvm::formatv("{0} {1}",
----------------
nit: `llvm::StringLiteral DirectiveSpelling = Directive ==
tooling::IncludeDirective::Include ? "Include" : "Import";` and use that inside
formatv calls
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:340
+ Edit = insert("\"header.h\"", tooling::IncludeDirective::Import);
+ EXPECT_TRUE(Edit);
+ EXPECT_EQ(Edit->newText, "#import \"header.h\"\n");
----------------
nit: ASSERT_TRUE, here and above.
================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:47
+enum IncludeDirective { Include, Import };
+
----------------
this will leak enum values to the whole `clang::tooling` namespace. can you
make this an `enum class IncludeDirective` instead (sorry for forgetting that
in the initial comment)
================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:77
+ insert(llvm::StringRef Header, bool IsAngled,
+ IncludeDirective Directive = IncludeDirective::Include) const;
----------------
i don't think we have many callers here. can we just update them instead of
defaulting to `Include` here?
================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:79
/// Removes all existing #includes of \p Header quoted with <> if \p IsAngled
/// is true or "" if \p IsAngled is false.
----------------
s/#includes/#includes and #imports/
================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:300
+ Offset, std::min(Line.size() + 1, Code.size() - Offset)),
+ Matches[1] == "include" ? tooling::IncludeDirective::Include
+ : tooling::IncludeDirective::Import),
----------------
i know this is not possible today, but just to be safe, can we actually check
for `import` here and default to `include` ?
================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:354
auto It = ExistingIncludes.find(IncludeName);
if (It != ExistingIncludes.end())
for (const auto &Inc : It->second)
----------------
nit: can you put some braces around the outermost `if`, as the statements
inside are getting long.
================
Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:378
assert(InsertOffset <= Code.size());
std::string NewInclude =
+ Directive == IncludeDirective::Include
----------------
nit:
```
llvm::StringLiteral DirectiveSpelling = Directive == IncludeDirective::Include
? "include" : "import";
std::string NewInclude = llvm::formatv("#{0} {1}\n", DirectiveSpelling,
QuotedName); // this already has an implicit conversion operator to std::string
```
================
Comment at: clang/unittests/Tooling/HeaderIncludesTest.cpp:65
+TEST_F(HeaderIncludesTest, InsertImportWithSameInclude) {
+ std::string Code = "#include \"a.h\"\n";
----------------
can you also add a removal test?
i know we didn't change the implementation, but today there's nothing checking
the behaviour. so explicitly encoding the expectation here, especially for the
case where we have both #include and #import with same spelling is useful. btw
is it likely that `#include "foo.h"` and `#import "foo.h"` can refer to
different physical headers (e.g. due to framework handling and whatnot)?
we're not changing the behaviour today, but if there's a difference, this is
likely a bug (that already exists), so we should leave a fixme (or address it
here if you got time, by introducing a IncludeDirective parameter into remove.
updating users is likely harder in this case, as everyone needs to
propagate/store the directive during their analysis).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128677/new/
https://reviews.llvm.org/D128677
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
