kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:456
+ auto IncludeStyle =
+ clang::format::getLLVMStyle().IncludeStyle;
+ auto DefaultStyle = clang::format::getStyle(
----------------
creating a copy of LLVM style unnecessarily all the time is not really great,
can you move this into the failure case instead?
also you can drop the `clang::` here and elsewhere, as this code is already
part of `clang::` namespace.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:457
+ clang::format::getLLVMStyle().IncludeStyle;
+ auto DefaultStyle = clang::format::getStyle(
+ clang::format::DefaultFormatStyle, MainFile->getName(),
----------------
as mentioned above we also need to make sure we're passing the relevant VFS
instance inside the source manager, rather than using the real file system (as
some clients rely on the VFS).
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:458
+ auto DefaultStyle = clang::format::getStyle(
+ clang::format::DefaultFormatStyle, MainFile->getName(),
+ clang::format::DefaultFallbackStyle);
----------------
s/MainFile->getName()/AST.tuPath()/
to be consistent with other places.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:460
+ clang::format::DefaultFallbackStyle);
+ if (!DefaultStyle.takeError()) {
+ IncludeStyle = DefaultStyle->IncludeStyle;
----------------
can you also `elog` this error? as it should be rare and when this goes wrong,
having this mentioned in the logs are really useful for debugging (since the
failure is actually outside of clangd, it usually means a malformed config file
somewhere)
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:426
+ void foo() {
+ ^b();
+ })cpp");
----------------
nit: instead of using a point, can you use a range here instead (i.e. `[[b]]`)?
afterwards you can have a `FileRange` pointing at both offsets, rather than
relying on the length of the identifier.
================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:446
+ BDecl = CandidateDecl;
+ include_cleaner::Symbol B{*D};
+ auto Offset = positionToOffset(MainFile.code(), MainFile.point());
----------------
rest of the code here doesn't really belong to the for loop, can you take them
out?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143496/new/
https://reviews.llvm.org/D143496
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
