[PATCH] D87775: [clangd] Add option for disabling AddUsing tweak on some namespaces.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!



================
Comment at: clang-tools-extra/clangd/Config.h:66
+
+  /// Style of the codebase.
+  struct {
----------------
describe elements more precisely?
They are namespaces with/without leading/trailing ::, and sub-namespaces are implicitly included.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:215
+      for (auto &N : F.FullyQualifiedNamespaces)
+        FullyQualifiedNamespaces.push_back(std::move(*N));
+      Out.Apply.push_back([FullyQualifiedNamespaces(
----------------
this is a good place to validate and/or normalize (leading/trailing ::)


================
Comment at: clang-tools-extra/clangd/ConfigFragment.h:167
+  struct StyleBlock {
+    // List of namespaces that should not appear in "using" declarations.
+    std::vector<Located<std::string>> FullyQualifiedNamespaces;
----------------
Can we describe this positively first?

Namespaces whose members should be fully-qualified, rather than via using declarations or directives.
Affects availability of the "add using declaration" tweak.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:197
+  std::string Buf;
+  llvm::raw_string_ostream NamespaceStream(Buf);
+  Namespace.print(NamespaceStream,
----------------
validate it's a namespace, and then call printNamespaceScope (AST.h)?

This handles the right spelling of anonymous/inline NSes.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:208
+    // that we do not prefix-match ::foo to ::foobar.
+    Banned.consume_front("::");
+    Banned.consume_back("::");
----------------
we should do this at config compile time instead I think - conceptually simpler if we store normalized data


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp:210
+    Banned.consume_back("::");
+    if (NamespaceStr.startswith(Banned.str() + "::"))
+      return true;
----------------
slightly silly, but can we take a stringref, consume Banned, and verify that the result is empty or begins with ::?

Avoids a silly string copy, explicitly handles the sub-namespace case, and is just more typical of the string code we write.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87775/new/

https://reviews.llvm.org/D87775