[clangd] Have visibleNamespaces() and getEligiblePoints() take a LangOptions rather than a FormatStyle

[nridge]

Summary

These functions only use the FormatStyle to obtain a LangOptions via
format::getFormattingLangOpts(), and some callers can more easily obtain
a LangOptions more directly.

[kadircet]

LGTM, thanks for doing this!

clang-tools-extra/clangd/CodeComplete.cpp

1378

nit: please inline

1776

nit: formatting change could you please revert ?

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

219

nit: formatting change could you please revert ?

455

nit: formatting change could you please revert ?

474

nit: formatting change could you please revert ?

574

nit: formatting change could you please revert ?

[nridge]

Address review comments

[hubert.reinterpretcast]

It looks like this is causing bot failures (http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/1881/steps/build%20stage%201/logs/stdio):

: && /home/buildbots/clang.9.0.0/bin/clang++ --gcc-toolchain=/opt/rh/devtoolset-7/root/usr -fPIC -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3  -Wl,-z,defs -Wl,-z,nodelete   -Wl,-rpath-link,/home/docker/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/stage1/./lib  -Wl,-O3 -Wl,--gc-sections -shared -Wl,-soname,libclangDaemonTweaks.so.11git -o lib/libclangDaemonTweaks.so.11git tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/AnnotateHighlightings.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/DumpAST.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/DefineInline.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/DefineOutline.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/ExpandAutoType.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/ExpandMacro.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/ExtractFunction.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/ExtractVariable.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/ObjCLocalizeStringLiteral.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/RawStringLiteral.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/RemoveUsingNamespace.cpp.o tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o  -Wl,-rpath,"\$ORIGIN/../lib" lib/libclangAST.so.11git lib/libclangBasic.so.11git lib/libclangDaemon.so.11git lib/libclangLex.so.11git lib/libclangToolingCore.so.11git lib/libclangToolingRefactoring.so.11git lib/libclangToolingSyntax.so.11git lib/libLLVMSupport.so.11git && :
tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/DefineOutline.cpp.o: In function `clang::clangd::(anonymous namespace)::DefineOutline::apply(clang::clangd::Tweak::Selection const&)':
DefineOutline.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_113DefineOutline5applyERKNS0_5Tweak9SelectionE+0x450): undefined reference to `clang::format::getFormattingLangOpts(clang::format::FormatStyle const&)'
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

[nridge]

In D75716#1916032, @hubert.reinterpretcast wrote:

It looks like this is causing bot failures (http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/1881/steps/build%20stage%201/logs/stdio):

Sorry about that. Fix here: https://reviews.llvm.org/D75969

[hubert.reinterpretcast]

In D75716#1916032, @hubert.reinterpretcast wrote:

It looks like this is causing bot failures (http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/1881/steps/build%20stage%201/logs/stdio):

Fix attempted in rG48121a5743b684def33d158391c5424626de28e2.

clang-tools-extra/clangd/CodeComplete.cpp

for (const auto &IDAndCount : Identifiers) {
       if (ID.References > 0)
         IdentifierResults.push_back(std::move(ID));
     }
 
     // Simplified version of getQueryScopes():
     //  - accessible scopes are determined heuristically.
     //  - all-scopes query if no qualifier was typed (and it's allowed).
     SpecifiedScope Scopes;
+    LangOptions LangOpts = format::getFormattingLangOpts(Style);
     Scopes.AccessibleScopes =
-        visibleNamespaces(Content.take_front(Offset), Style);
+        visibleNamespaces(Content.take_front(Offset), LangOpts);
     for (std::string &S : Scopes.AccessibleScopes)
       if (!S.empty())
         S.append("::"); // visibleNamespaces doesn't include trailing ::.
     if (HeuristicPrefix.Qualifier.empty())
       AllScopes = Opts.AllScopes;
     else if (HeuristicPrefix.Qualifier.startswith("::")) {
       Scopes.AccessibleScopes = {""};
       Scopes.UnresolvedQualifier =
SignatureHelp signatureHelp(PathRef FileName,
   SignatureHelp Result;
   clang::CodeCompleteOptions Options;
   Options.IncludeGlobals = false;
   Options.IncludeMacros = false;
   Options.IncludeCodePatterns = false;
   Options.IncludeBriefComments = false;
   IncludeStructure PreambleInclusions; // Unused for signatureHelp
   semaCodeComplete(
-      std::make_unique<SignatureHelpCollector>(Options, Index, Result),
-      Options,
+      std::make_unique<SignatureHelpCollector>(Options, Index, Result), Options,
       {FileName, Command, Preamble, Contents, *Offset, std::move(VFS)});
   return Result;
 }
 
 bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx) {
   auto InTopLevelScope = [](const NamedDecl &ND) {
     switch (ND.getDeclContext()->getDeclKind()) {
     case Decl::TranslationUnit:

clang-tools-extra/clangd/SourceCode.h

 ///
 /// example:
 ///   using namespace a;
 ///   namespace foo {
 ///     using namespace b;
 ///
 /// visibleNamespaces are {"foo::", "", "a::", "b::", "foo::b::"}, not "a::b::".
 std::vector<std::string> visibleNamespaces(llvm::StringRef Code,
-                                           const format::FormatStyle &Style);
+                                           const LangOptions &LangOpts);
 
 /// Represents locations that can accept a definition.
 struct EligibleRegion {
   /// Namespace that owns all of the EligiblePoints, e.g.
   /// namespace a{ namespace b {^ void foo();^} }
   /// It will be “a::b” for both carrot locations.
   std::string EnclosingNamespace;
   /// Offsets into the code marking eligible points to insert a function
   /// definition.
   std::vector<Position> EligiblePoints;
 };
 
 /// Returns most eligible region to insert a definition for \p
 /// FullyQualifiedName in the \p Code.
 /// Pseudo parses \pCode under the hood to determine namespace decls and
 /// possible insertion points. Choses the region that matches the longest prefix
 /// of \p FullyQualifiedName. Returns EOF if there are no shared namespaces.
 /// \p FullyQualifiedName should not contain anonymous namespaces.
 EligibleRegion getEligiblePoints(llvm::StringRef Code,
                                  llvm::StringRef FullyQualifiedName,
-                                 const format::FormatStyle &Style);
+                                 const LangOptions &LangOpts);
 
 struct DefinedMacro {
   llvm::StringRef Name;
   const MacroInfo *Info;
 };
 /// Gets the macro referenced by \p SpelledTok. It must be a spelled token
 /// aligned to the beginning of an identifier.
 llvm::Optional<DefinedMacro> locateMacroAt(const syntax::Token &SpelledTok,

clang-tools-extra/clangd/SourceCode.cpp

enum {
     EndNamespace,   // } // namespace <ns>.  Payload is resolved *outer*
                     // namespace.
     UsingDirective  // using namespace <ns>. Payload is unresolved <ns>.
   } Trigger;
   std::string Payload;
   Position Pos;
 };
 // Scans C++ source code for constructs that change the visible namespaces.
-void parseNamespaceEvents(llvm::StringRef Code,
-                          const format::FormatStyle &Style,
+void parseNamespaceEvents(llvm::StringRef Code, const LangOptions &LangOpts,
                           llvm::function_ref<void(NamespaceEvent)> Callback) {
 
   // Stack of enclosing namespaces, e.g. {"clang", "clangd"}
   std::vector<std::string> Enclosing; // Contains e.g. "clang", "clangd"
   // Stack counts open braces. true if the brace opened a namespace.
   std::vector<bool> BraceStack;
 
   enum {
     Default,
     Namespace,          // just saw 'namespace'
     NamespaceName,      // just saw 'namespace' NSName
     Using,              // just saw 'using'
     UsingNamespace,     // just saw 'using namespace'
     UsingNamespaceName, // just saw 'using namespace' NSName
   } State = Default;
   std::string NSName;
 
   NamespaceEvent Event;
-  lex(Code, format::getFormattingLangOpts(Style),
-      [&](const syntax::Token &Tok, const SourceManager &SM) {
-        Event.Pos = sourceLocToPosition(SM, Tok.location());
-        switch (Tok.kind()) {
-        case tok::kw_using:
-          State = State == Default ? Using : Default;
-          break;
-        case tok::kw_namespace:
-          switch (State) {
-          case Using:
-            State = UsingNamespace;
-            break;
-          case Default:
-            State = Namespace;
-            break;
-          default:
-            State = Default;
-            break;
-          }
-          break;
-        case tok::identifier:
-          switch (State) {
-          case UsingNamespace:
-            NSName.clear();
-            LLVM_FALLTHROUGH;
-          case UsingNamespaceName:
-            NSName.append(Tok.text(SM).str());
-            State = UsingNamespaceName;
-            break;
-          case Namespace:
-            NSName.clear();
-            LLVM_FALLTHROUGH;
-          case NamespaceName:
-            NSName.append(Tok.text(SM).str());
-            State = NamespaceName;
-            break;
-          case Using:
-          case Default:
-            State = Default;
-            break;
-          }
-          break;
-        case tok::coloncolon:
-          // This can come at the beginning or in the middle of a namespace
-          // name.
-          switch (State) {
-          case UsingNamespace:
-            NSName.clear();
-            LLVM_FALLTHROUGH;
-          case UsingNamespaceName:
-            NSName.append("::");
-            State = UsingNamespaceName;
-            break;
-          case NamespaceName:
-            NSName.append("::");
-            State = NamespaceName;
-            break;
-          case Namespace: // Not legal here.
-          case Using:
-          case Default:
-            State = Default;
-            break;
-          }
-          break;
-        case tok::l_brace:
-          // Record which { started a namespace, so we know when } ends one.
-          if (State == NamespaceName) {
-            // Parsed: namespace <name> {
-            BraceStack.push_back(true);
-            Enclosing.push_back(NSName);
-            Event.Trigger = NamespaceEvent::BeginNamespace;
-            Event.Payload = llvm::join(Enclosing, "::");
-            Callback(Event);
-          } else {
-            // This case includes anonymous namespaces (State = Namespace).
-            // For our purposes, they're not namespaces and we ignore them.
-            BraceStack.push_back(false);
-          }
-          State = Default;
-          break;
-        case tok::r_brace:
-          // If braces are unmatched, we're going to be confused, but don't
-          // crash.
-          if (!BraceStack.empty()) {
-            if (BraceStack.back()) {
-              // Parsed: } // namespace
-              Enclosing.pop_back();
-              Event.Trigger = NamespaceEvent::EndNamespace;
-              Event.Payload = llvm::join(Enclosing, "::");
-              Callback(Event);
-            }
-            BraceStack.pop_back();
-          }
-          break;
-        case tok::semi:
-          if (State == UsingNamespaceName) {
-            // Parsed: using namespace <name> ;
-            Event.Trigger = NamespaceEvent::UsingDirective;
-            Event.Payload = std::move(NSName);
-            Callback(Event);
-          }
-          State = Default;
-          break;
-        default:
-          State = Default;
-          break;
-        }
-      });
+  lex(Code, LangOpts, [&](const syntax::Token &Tok, const SourceManager &SM) {
 }
 
 // Returns the prefix namespaces of NS: {"" ... NS}.
 llvm::SmallVector<llvm::StringRef, 8> ancestorNamespaces(llvm::StringRef NS) {
   llvm::SmallVector<llvm::StringRef, 8> Results;
   Results.push_back(NS.take_front(0));
   NS.split(Results, "::", /*MaxSplit=*/-1, /*KeepEmpty=*/false);
   for (llvm::StringRef &R : Results)
     R = NS.take_front(R.end() - NS.begin());
   return Results;
 }
 
 } // namespace
 
 std::vector<std::string> visibleNamespaces(llvm::StringRef Code,
-                                           const format::FormatStyle &Style) {
+                                           const LangOptions &LangOpts) {
   std::string Current;
   // Map from namespace to (resolved) namespaces introduced via using directive.
   llvm::StringMap<llvm::StringSet<>> UsingDirectives;
 
-  parseNamespaceEvents(Code, Style, [&](NamespaceEvent Event) {
+  parseNamespaceEvents(Code, LangOpts, [&](NamespaceEvent Event) {
     llvm::StringRef NS = Event.Payload;
     switch (Event.Trigger) {
     case NamespaceEvent::BeginNamespace:
     case NamespaceEvent::EndNamespace:
       Current = std::move(Event.Payload);
       break;
     case NamespaceEvent::UsingDirective:
       if (NS.consume_front("::"))
if (auto NewEdits = cleanupAndFormat(E.InitialCode, E.Replacements, Style))
     E.Replacements = std::move(*NewEdits);
   else
     return NewEdits.takeError();
   return llvm::Error::success();
 }
 
 EligibleRegion getEligiblePoints(llvm::StringRef Code,
                                  llvm::StringRef FullyQualifiedName,
-                                 const format::FormatStyle &Style) {
+                                 const LangOptions &LangOpts) {
   EligibleRegion ER;
   // Start with global namespace.
   std::vector<std::string> Enclosing = {""};
   // FIXME: In addition to namespaces try to generate events for function
   // definitions as well. One might use a closing parantheses(")" followed by an
   // opening brace "{" to trigger the start.
-  parseNamespaceEvents(Code, Style, [&](NamespaceEvent Event) {
+  parseNamespaceEvents(Code, LangOpts, [&](NamespaceEvent Event) {
     // Using Directives only introduces declarations to current scope, they do
     // not change the current namespace, so skip them.
     if (Event.Trigger == NamespaceEvent::UsingDirective)
       return;
     // Do not qualify the global namespace.
     if (!Event.Payload.empty())
       Event.Payload.append("::");
 

clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp

struct InsertionPoint {
   std::string EnclosingNamespace;
   size_t Offset;
 };
 // Returns the most natural insertion point for \p QualifiedName in \p Contents.
 // This currently cares about only the namespace proximity, but in feature it
 // should also try to follow ordering of declarations. For example, if decls
 // come in order `foo, bar, baz` then this function should return some point
 // between foo and baz for inserting bar.
-llvm::Expected<InsertionPoint>
-getInsertionPoint(llvm::StringRef Contents, llvm::StringRef QualifiedName,
-                  const format::FormatStyle &Style) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, Style);
+llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
+                                                 llvm::StringRef QualifiedName,
+                                                 const LangOptions &LangOpts) {
+  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
 
   assert(!Region.EligiblePoints.empty());
   // FIXME: This selection can be made smarter by looking at the definition
   // locations for adjacent decls to Source. Unfortunately psudeo parsing in
   // getEligibleRegions only knows about namespace begin/end events so we
   // can't match function start/end positions yet.
   auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
   if (!Offset)
auto &FS =
         Sel.AST->getSourceManager().getFileManager().getVirtualFileSystem();
     auto Buffer = FS.getBufferForFile(*CCFile);
     // FIXME: Maybe we should consider creating the implementation file if it
     // doesn't exist?
     if (!Buffer)
       return llvm::createStringError(Buffer.getError(),
                                      Buffer.getError().message());
     auto Contents = Buffer->get()->getBuffer();
-    auto InsertionPoint =
-        getInsertionPoint(Contents, Source->getQualifiedNameAsString(),
-                          getFormatStyleForFile(*CCFile, Contents, &FS));
+    auto LangOpts = format::getFormattingLangOpts(
+    auto InsertionPoint = getInsertionPoint(
+        Contents, Source->getQualifiedNameAsString(), LangOpts);
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
     auto FuncDef = getFunctionSourceCode(
         Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens());
     if (!FuncDef)
       return FuncDef.takeError();
 

clang-tools-extra/clangd/unittests/SourceCodeTests.cpp

TEST(SourceCodeTests, PositionToOffset) {
   EXPECT_THAT_EXPECTED(positionToOffset(File, position(-1, 2)), llvm::Failed());
   EXPECT_THAT_EXPECTED(positionToOffset(File, position(3, 0)), llvm::Failed());
   for (Line L : FileLines) {
     EXPECT_THAT_EXPECTED(positionToOffset(File, position(L.Number, -1)),
                          llvm::Failed()); // out of range
     for (unsigned I = 0; I <= L.Length; ++I)
       EXPECT_THAT_EXPECTED(positionToOffset(File, position(L.Number, I)),
                            llvm::HasValue(L.Offset + I));
-    EXPECT_THAT_EXPECTED(positionToOffset(File, position(L.Number, L.Length+1)),
-                         llvm::HasValue(L.Offset + L.Length));
+    EXPECT_THAT_EXPECTED(
+        positionToOffset(File, position(L.Number, L.Length + 1)),
     EXPECT_THAT_EXPECTED(
         positionToOffset(File, position(L.Number, L.Length + 1), false),
         llvm::Failed()); // out of range
   }
 }
 
 TEST(SourceCodeTests, OffsetToPosition) {
   EXPECT_THAT(offsetToPosition(File, 0), Pos(0, 0)) << "start of file";
std::vector<std::pair<const char *, std::vector<std::string>>> Cases = {
       },
       {
           R"cpp(
             // Parse until EOF
             namespace bar{})cpp",
           {""},
       },
   };
-  for (const auto& Case : Cases) {
+  for (const auto &Case : Cases) {
     EXPECT_EQ(Case.second,
-              visibleNamespaces(Case.first, format::getLLVMStyle()))
+              visibleNamespaces(Case.first, format::getFormattingLangOpts(
+                                                format::getLLVMStyle())))
         << Case.first;
   }
 }
 
 TEST(SourceCodeTests, GetMacros) {
   Annotations Code(R"cpp(
      #define MACRO 123
      int abc = MA^CRO;
TEST(SourceCodeTests, WorksAtBeginOfFile) {
   ASSERT_TRUE(bool(CurLoc));
   const auto *Id = syntax::spelledIdentifierTouching(*CurLoc, AST.getTokens());
   ASSERT_TRUE(Id);
   auto Result = locateMacroAt(*Id, AST.getPreprocessor());
   ASSERT_TRUE(Result);
   EXPECT_THAT(*Result, MacroName("MACRO"));
 }
 
-TEST(SourceCodeTests, IsInsideMainFile){
+TEST(SourceCodeTests, IsInsideMainFile) {
   TestTU TU;
   TU.HeaderCode = R"cpp(
     #define DEFINE_CLASS(X) class X {};
     #define DEFINE_YY DEFINE_CLASS(YY)
 
     class Header1 {};
     DEFINE_CLASS(Header2)
     class Header {};
   )cpp";
   TU.Code = R"cpp(
     class Main1 {};
     DEFINE_CLASS(Main2)
     DEFINE_YY
     class Main {};
   )cpp";
   TU.ExtraArgs.push_back("-DHeader=Header3");
   TU.ExtraArgs.push_back("-DMain=Main3");
   auto AST = TU.build();
-  const auto& SM = AST.getSourceManager();
+  const auto &SM = AST.getSourceManager();
   auto DeclLoc = [&AST](llvm::StringRef Name) {
     return findDecl(AST, Name).getLocation();
   };
   for (const auto *HeaderDecl : {"Header1", "Header2", "Header3"})
     EXPECT_FALSE(isInsideMainFile(DeclLoc(HeaderDecl), SM));
 
   for (const auto *MainDecl : {"Main1", "Main2", "Main3", "YY"})
     EXPECT_TRUE(isInsideMainFile(DeclLoc(MainDecl), SM));

 #define HEADER "bar.inc"
   $bar^#  include HEADER
   )cpp";
   Annotations Test(Case);
   auto TU = TestTU::withCode(Test.code());
   TU.AdditionalFiles["foo.inc"] = "int foo;\n";
   TU.AdditionalFiles["bar.inc"] = "int bar;\n";
   auto AST = TU.build();
-  const auto& SM = AST.getSourceManager();
+  const auto &SM = AST.getSourceManager();
 
   FileID Foo = SM.getFileID(findDecl(AST, "foo").getLocation());
   EXPECT_EQ(SM.getFileOffset(includeHashLoc(Foo, SM)),
             Test.llvm::Annotations::point("foo"));
   FileID Bar = SM.getFileID(findDecl(AST, "bar").getLocation());
   EXPECT_EQ(SM.getFileOffset(includeHashLoc(Bar, SM)),
             Test.llvm::Annotations::point("bar"));
 }
const char *EnclosingNamespace;
               namespace ns {}
               ^}
               namespace ns1 {^namespace ns {}^})cpp",
        "ns1::ns2::symbol", "ns1::"},
   };
   for (auto Case : Cases) {
     Annotations Test(Case.Code);
 
-    auto Res = getEligiblePoints(Test.code(), Case.FullyQualifiedName,
-                                 format::getLLVMStyle());
+    auto Res = getEligiblePoints(
+        Test.code(), Case.FullyQualifiedName,
+        format::getFormattingLangOpts(format::getLLVMStyle()));
     EXPECT_THAT(Res.EligiblePoints, testing::ElementsAreArray(Test.points()))
         << Test.code();
     EXPECT_EQ(Res.EnclosingNamespace, Case.EnclosingNamespace) << Test.code();
   }
 }
 
 TEST(SourceCodeTests, IdentifierRanges) {
   Annotations Code(R"cpp(