kadircet added a comment.
we should have tests in `clang/unittests/Tooling/HeaderIncludesTest.cpp` and
the commit itself should be tagged as `[clang][Tooling]` rather than `[clangd]`.
================
Comment at: clang-tools-extra/clangd/Headers.h:250
+ llvm::Optional<TextEdit> insert(llvm::StringRef VerbatimHeader,
+ bool ViaImport) const;
----------------
again it's better to have an enum here.
================
Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:253
+ llvm::StringRef Symbol,
+ bool ViaImport) const {
Fix F;
----------------
again can we rather pass the directive enum around?
================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:74
+ llvm::Optional<tooling::Replacement>
+ insert(llvm::StringRef Header, bool IsAngled, bool IsImport = false) const;
----------------
rather than a boolean flag, can you introduce an `enum IncludeDirective {
Include, Import };` and default it to `Include` here?
================
Comment at: clang/include/clang/Tooling/Inclusions/HeaderIncludes.h:86
private:
struct Include {
Include(StringRef Name, tooling::Range R) : Name(Name), R(R) {}
----------------
we also need to preserve includedirective here now. as the behaviour of
`insert` is to return none iff it's exactly the same spelling. so if there's a
`#include "foo.h"` and we want to insert `#import "foo.h"` it shouldn't fail.
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
