[clang-tools-extra] r344672 - [clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction. (re-land r344620)

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Oct 17 00:32:05 2018
New Revision: 344672

URL: http://llvm.org/viewvc/llvm-project?rev=344672&view=rev
Log:
[clangd] Refactor JSON-over-stdin/stdout code into Transport abstraction. 
(re-land r344620)

Summary:
This paves the way for alternative transports (mac XPC, maybe messagepack?),
and also generally improves layering: testing ClangdLSPServer becomes less of
a pipe dream, we split up the JSONOutput monolith, etc.

This isn't a final state, much of what remains in JSONRPCDispatcher can go away,
handlers can call reply() on the transport directly, JSONOutput can be renamed
to StreamLogger and removed, etc. But this patch is sprawling already.

The main observable change (see tests) is that hitting EOF on input is now an
error: the client should send the 'exit' notification.
This is defensible: the protocol doesn't spell this case out. Reproducing the
current behavior for all combinations of shutdown/exit/EOF clutters interfaces.
We can iterate on this if desired.

Reviewers: jkorous, ioeric, hokein

Subscribers: mgorny, ilya-biryukov, MaskRay, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D53286

Added:
clang-tools-extra/trunk/clangd/JSONTransport.cpp
clang-tools-extra/trunk/clangd/Transport.h
clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp
Modified:
clang-tools-extra/trunk/clangd/CMakeLists.txt
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/clangd/ProtocolHandlers.cpp
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
clang-tools-extra/trunk/test/clangd/completion-snippets.test
clang-tools-extra/trunk/test/clangd/completion.test
clang-tools-extra/trunk/test/clangd/crash-non-added-files.test
clang-tools-extra/trunk/test/clangd/execute-command.test
clang-tools-extra/trunk/test/clangd/input-mirror.test
clang-tools-extra/trunk/test/clangd/signature-help.test
clang-tools-extra/trunk/test/clangd/textdocument-didchange-fail.test
clang-tools-extra/trunk/test/clangd/trace.test
clang-tools-extra/trunk/test/clangd/xrefs.test
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

Modified: clang-tools-extra/trunk/clangd/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CMakeLists.txt?rev=344672&r1=344671&r2=344672&view=diff
==
--- clang-tools-extra/trunk/clangd/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clangd/CMakeLists.txt Wed Oct 17 00:32:05 2018
@@ -26,6 +26,7 @@ add_clang_library(clangDaemon
   GlobalCompilationDatabase.cpp
   Headers.cpp
   JSONRPCDispatcher.cpp
+  JSONTransport.cpp
   Logger.cpp
   Protocol.cpp
   ProtocolHandlers.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=344672&r1=344671&r2=344672&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Oct 17 00:32:05 2018
@@ -162,7 +162,10 @@ void ClangdLSPServer::onShutdown(Shutdow
   reply(nullptr);
 }
 
-void ClangdLSPServer::onExit(ExitParams &Params) { IsDone = true; }
+void ClangdLSPServer::onExit(ExitParams &Params) {
+  // No work to do.
+  // JSONRPCDispatcher shuts down the transport after this notification.
+}
 
 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
@@ -497,39 +500,41 @@ void ClangdLSPServer::onReference(Refere
  });
 }
 
-ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
+ClangdLSPServer::ClangdLSPServer(class Transport &Transp,
  const clangd::CodeCompleteOptions &CCOpts,
  llvm::Optional CompileCommandsDir,
  bool ShouldUseInMemoryCDB,
  const ClangdServer::Options &Opts)
-: Out(Out), CDB(ShouldUseInMemoryCDB ? CompilationDB::makeInMemory()
- : CompilationDB::makeDirectoryBased(
-   std::move(CompileCommandsDir))),
+: Transp(Transp),
+  CDB(ShouldUseInMemoryCDB ? CompilationDB::makeInMemory()
+   : CompilationDB::makeDirectoryBased(
+ std::move(CompileCommandsDir))),
   CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
   Sup

[clang-tools-extra] r344673 - [clangd] Simplify client capabilities parsing.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Oct 17 00:33:42 2018
New Revision: 344673

URL: http://llvm.org/viewvc/llvm-project?rev=344673&view=rev
Log:
[clangd] Simplify client capabilities parsing.

Summary:
Instead of parsing into structs that mirror LSP, simply parse into a flat struct
that contains the info we need.
This is an exception to our strategy with Protocol.h, which seems justified:
 - the structure here is very large and deeply nested
 - we care about almost none of it
 - we should never have to serialize client capabilities

Reviewers: kadircet

Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, cfe-commits

Differential Revision: https://reviews.llvm.org/D53266

Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=344673&r1=344672&r2=344673&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Oct 17 00:33:42 2018
@@ -97,29 +97,14 @@ void ClangdLSPServer::onInitialize(Initi
   else if (Params.rootPath && !Params.rootPath->empty())
 Server->setRootPath(*Params.rootPath);
 
-  CCOpts.EnableSnippets =
-  
Params.capabilities.textDocument.completion.completionItem.snippetSupport;
-  DiagOpts.EmbedFixesInDiagnostics =
-  Params.capabilities.textDocument.publishDiagnostics.clangdFixSupport;
-  DiagOpts.SendDiagnosticCategory =
-  Params.capabilities.textDocument.publishDiagnostics.categorySupport;
-  SupportsCodeAction =
-  Params.capabilities.textDocument.codeActionLiteralSupport;
-
-  if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
-  Params.capabilities.workspace->symbol->symbolKind &&
-  Params.capabilities.workspace->symbol->symbolKind->valueSet) {
-for (SymbolKind Kind :
- *Params.capabilities.workspace->symbol->symbolKind->valueSet) {
-  SupportedSymbolKinds.set(static_cast(Kind));
-}
-  }
-
-  if (Params.capabilities.textDocument.completion.completionItemKind &&
-  Params.capabilities.textDocument.completion.completionItemKind->valueSet)
-for (CompletionItemKind Kind : *Params.capabilities.textDocument.completion
-.completionItemKind->valueSet)
-  SupportedCompletionItemKinds.set(static_cast(Kind));
+  CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets;
+  DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes;
+  DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory;
+  if (Params.capabilities.WorkspaceSymbolKinds)
+SupportedSymbolKinds |= *Params.capabilities.WorkspaceSymbolKinds;
+  if (Params.capabilities.CompletionItemKinds)
+SupportedCompletionItemKinds |= *Params.capabilities.CompletionItemKinds;
+  SupportsCodeAction = Params.capabilities.CodeActionStructure;
 
   reply(json::Object{
   {{"capabilities",

Modified: clang-tools-extra/trunk/clangd/Protocol.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Protocol.cpp?rev=344673&r1=344672&r2=344673&view=diff
==
--- clang-tools-extra/trunk/clangd/Protocol.cpp (original)
+++ clang-tools-extra/trunk/clangd/Protocol.cpp Wed Oct 17 00:33:42 2018
@@ -161,35 +161,6 @@ bool fromJSON(const json::Value &E, Trac
   return false;
 }
 
-bool fromJSON(const json::Value &Params, CompletionItemClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("snippetSupport", R.snippetSupport);
-  O.map("commitCharacterSupport", R.commitCharacterSupport);
-  return true;
-}
-
-bool fromJSON(const json::Value &Params, CompletionClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("dynamicRegistration", R.dynamicRegistration);
-  O.map("completionItem", R.completionItem);
-  O.map("contextSupport", R.contextSupport);
-  return true;
-}
-
-bool fromJSON(const llvm::json::Value &Params,
-  PublishDiagnosticsClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("clangdFixSupport", R.clangdFixSupport);
-  O.map("categorySupport", R.categorySupport);
-  return true;
-}
-
 bool fromJSON(const json::Value &E, SymbolKind &Out) {
   if (auto T = E.getAsInteger()) {
 if (*T < static_cast(SymbolKind::File) ||
@@ -201,24 +172,18 @@ bool fromJSON(const json::Value &E, Symb
   return false;
 }
 
-bool fromJSON(const json::Value &E, std::vector &Out) {
+bool fromJSON(const json::Value &E, SymbolKindBitset &Out) {
   if (auto *A = E.getAsArray()) {
-Out.clear();
 for (size_t I = 0; I < A->size(); ++I) {
   SymbolKind Kind

[PATCH] D53266: [clangd] Simplify client capabilities parsing.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344673: [clangd] Simplify client capabilities parsing. 
(authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D53266

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -97,29 +97,14 @@
   else if (Params.rootPath && !Params.rootPath->empty())
 Server->setRootPath(*Params.rootPath);
 
-  CCOpts.EnableSnippets =
-  Params.capabilities.textDocument.completion.completionItem.snippetSupport;
-  DiagOpts.EmbedFixesInDiagnostics =
-  Params.capabilities.textDocument.publishDiagnostics.clangdFixSupport;
-  DiagOpts.SendDiagnosticCategory =
-  Params.capabilities.textDocument.publishDiagnostics.categorySupport;
-  SupportsCodeAction =
-  Params.capabilities.textDocument.codeActionLiteralSupport;
-
-  if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
-  Params.capabilities.workspace->symbol->symbolKind &&
-  Params.capabilities.workspace->symbol->symbolKind->valueSet) {
-for (SymbolKind Kind :
- *Params.capabilities.workspace->symbol->symbolKind->valueSet) {
-  SupportedSymbolKinds.set(static_cast(Kind));
-}
-  }
-
-  if (Params.capabilities.textDocument.completion.completionItemKind &&
-  Params.capabilities.textDocument.completion.completionItemKind->valueSet)
-for (CompletionItemKind Kind : *Params.capabilities.textDocument.completion
-.completionItemKind->valueSet)
-  SupportedCompletionItemKinds.set(static_cast(Kind));
+  CCOpts.EnableSnippets = Params.capabilities.CompletionSnippets;
+  DiagOpts.EmbedFixesInDiagnostics = Params.capabilities.DiagnosticFixes;
+  DiagOpts.SendDiagnosticCategory = Params.capabilities.DiagnosticCategory;
+  if (Params.capabilities.WorkspaceSymbolKinds)
+SupportedSymbolKinds |= *Params.capabilities.WorkspaceSymbolKinds;
+  if (Params.capabilities.CompletionItemKinds)
+SupportedCompletionItemKinds |= *Params.capabilities.CompletionItemKinds;
+  SupportsCodeAction = Params.capabilities.CodeActionStructure;
 
   reply(json::Object{
   {{"capabilities",
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -161,35 +161,6 @@
   return false;
 }
 
-bool fromJSON(const json::Value &Params, CompletionItemClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("snippetSupport", R.snippetSupport);
-  O.map("commitCharacterSupport", R.commitCharacterSupport);
-  return true;
-}
-
-bool fromJSON(const json::Value &Params, CompletionClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("dynamicRegistration", R.dynamicRegistration);
-  O.map("completionItem", R.completionItem);
-  O.map("contextSupport", R.contextSupport);
-  return true;
-}
-
-bool fromJSON(const llvm::json::Value &Params,
-  PublishDiagnosticsClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  if (!O)
-return false;
-  O.map("clangdFixSupport", R.clangdFixSupport);
-  O.map("categorySupport", R.categorySupport);
-  return true;
-}
-
 bool fromJSON(const json::Value &E, SymbolKind &Out) {
   if (auto T = E.getAsInteger()) {
 if (*T < static_cast(SymbolKind::File) ||
@@ -201,24 +172,18 @@
   return false;
 }
 
-bool fromJSON(const json::Value &E, std::vector &Out) {
+bool fromJSON(const json::Value &E, SymbolKindBitset &Out) {
   if (auto *A = E.getAsArray()) {
-Out.clear();
 for (size_t I = 0; I < A->size(); ++I) {
   SymbolKind KindOut;
   if (fromJSON((*A)[I], KindOut))
-Out.push_back(KindOut);
+Out.set(size_t(KindOut));
 }
 return true;
   }
   return false;
 }
 
-bool fromJSON(const json::Value &Params, SymbolKindCapabilities &R) {
-  json::ObjectMapper O(Params);
-  return O && O.map("valueSet", R.valueSet);
-}
-
 SymbolKind adjustKindToCapability(SymbolKind Kind,
   SymbolKindBitset &SupportedSymbolKinds) {
   auto KindVal = static_cast(Kind);
@@ -237,34 +202,46 @@
   }
 }
 
-bool fromJSON(const json::Value &Params, WorkspaceSymbolCapabilities &R) {
-  json::ObjectMapper O(Params);
-  return O && O.map("symbolKind", R.symbolKind);
-}
-
-bool fromJSON(const json::Value &Params, WorkspaceClientCapabilities &R) {
-  json::ObjectMapper O(Params);
-  return O && O.map("symbol", R.symbol);
-}
-
-bool fromJSON(const json::Value &Params, TextDocumentCl

[clang-tools-extra] r344675 - [clangd] Rename and move trivial logger to Logger.cpp. NFC

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Oct 17 00:39:32 2018
New Revision: 344675

URL: http://llvm.org/viewvc/llvm-project?rev=344675&view=rev
Log:
[clangd] Rename and move trivial logger to Logger.cpp. NFC

Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
clang-tools-extra/trunk/clangd/Logger.cpp
clang-tools-extra/trunk/clangd/Logger.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=344675&r1=344674&r2=344675&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Wed Oct 17 00:39:32 2018
@@ -24,7 +24,6 @@
 namespace clang {
 namespace clangd {
 
-class JSONOutput;
 class SymbolIndex;
 
 /// This class exposes ClangdServer's capabilities via Language Server 
Protocol.

Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=344675&r1=344674&r2=344675&view=diff
==
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp (original)
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp Wed Oct 17 00:39:32 
2018
@@ -15,9 +15,7 @@
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/Support/Chrono.h"
 #include "llvm/Support/Errno.h"
-#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/SourceMgr.h"
@@ -59,18 +57,6 @@ public:
 Key> RequestSpan::RSKey;
 } // namespace
 
-void JSONOutput::log(Logger::Level Level,
- const llvm::formatv_object_base &Message) {
-  if (Level < MinLevel)
-return;
-  llvm::sys::TimePoint<> Timestamp = std::chrono::system_clock::now();
-  trace::log(Message);
-  std::lock_guard Guard(StreamMutex);
-  Logs << llvm::formatv("{0}[{1:%H:%M:%S.%L}] {2}\n", indicator(Level),
-Timestamp, Message);
-  Logs.flush();
-}
-
 void clangd::reply(json::Value &&Result) {
   auto ID = getRequestId();
   if (!ID) {

Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h?rev=344675&r1=344674&r2=344675&view=diff
==
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h (original)
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h Wed Oct 17 00:39:32 2018
@@ -25,23 +25,6 @@
 namespace clang {
 namespace clangd {
 
-// Logs to an output stream, such as stderr.
-// FIXME: Rename to StreamLogger or such, and move to Logger.h.
-class JSONOutput : public Logger {
-public:
-  JSONOutput(llvm::raw_ostream &Logs, Logger::Level MinLevel)
-  : MinLevel(MinLevel), Logs(Logs) {}
-
-  /// Write a line to the logging stream.
-  void log(Level, const llvm::formatv_object_base &Message) override;
-
-private:
-  Logger::Level MinLevel;
-  llvm::raw_ostream &Logs;
-
-  std::mutex StreamMutex;
-};
-
 /// Sends a successful reply.
 /// Current context must derive from JSONRPCDispatcher::Handler.
 void reply(llvm::json::Value &&Result);

Modified: clang-tools-extra/trunk/clangd/Logger.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Logger.cpp?rev=344675&r1=344674&r2=344675&view=diff
==
--- clang-tools-extra/trunk/clangd/Logger.cpp (original)
+++ clang-tools-extra/trunk/clangd/Logger.cpp Wed Oct 17 00:39:32 2018
@@ -8,6 +8,9 @@
 
//===--===//
 
 #include "Logger.h"
+#include "Trace.h"
+#include "llvm/Support/Chrono.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 
@@ -44,5 +47,17 @@ const char *detail::debugType(const char
   return Filename;
 }
 
+void StreamLogger::log(Logger::Level Level,
+   const llvm::formatv_object_base &Message) {
+  if (Level < MinLevel)
+return;
+  llvm::sys::TimePoint<> Timestamp = std::chrono::system_clock::now();
+  trace::log(Message);
+  std::lock_guard Guard(StreamMutex);
+  Logs << llvm::formatv("{0}[{1:%H:%M:%S.%L}] {2}\n", indicator(Level),
+Timestamp, Message);
+  Logs.flush();
+}
+
 } // namespace clangd
 } // namespace clang

Modified: clang-tools-extra/trunk/clangd/Logger.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Logger.h?rev=344675&r1=344674&r2=344675&view=diff
=

[clang-tools-extra] r344676 - [clangd] Hide unused function. NFC

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Oct 17 00:41:53 2018
New Revision: 344676

URL: http://llvm.org/viewvc/llvm-project?rev=344676&view=rev
Log:
[clangd] Hide unused function. NFC

Modified:
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h

Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=344676&r1=344675&r2=344676&view=diff
==
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp (original)
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp Wed Oct 17 00:41:53 
2018
@@ -58,7 +58,7 @@ Key> Reques
 } // namespace
 
 void clangd::reply(json::Value &&Result) {
-  auto ID = getRequestId();
+  auto ID = Context::current().get(RequestID);
   if (!ID) {
 elog("Attempted to reply to a notification!");
 return;
@@ -77,7 +77,7 @@ void clangd::replyError(ErrorCode Code,
  {"message", Message.str()}};
   });
 
-  if (auto ID = getRequestId()) {
+  if (auto ID = Context::current().get(RequestID)) {
 log("--> reply({0}) error: {1}", *ID, Message);
 Context::current()
 .getExisting(CurrentTransport)
@@ -206,7 +206,3 @@ llvm::Error JSONRPCDispatcher::runLangua
   WithContextValue WithTransport(CurrentTransport, &Transport);
   return Transport.loop(*this);
 }
-
-const json::Value *clangd::getRequestId() {
-  return Context::current().get(RequestID);
-}

Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h?rev=344676&r1=344675&r2=344676&view=diff
==
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h (original)
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.h Wed Oct 17 00:41:53 2018
@@ -35,9 +35,6 @@ void replyError(ErrorCode Code, const ll
 /// fetches the related message from error's message method. If error doesn't
 /// match any known errors, uses ErrorCode::InvalidParams for the error.
 void replyError(llvm::Error E);
-/// Returns the request-id of the current request. Should not be used directly
-/// for replying to requests, use the above mentioned methods for that case.
-const llvm::json::Value *getRequestId();
 /// Sends a request to the client.
 /// Current context must derive from JSONRPCDispatcher::Handler.
 void call(llvm::StringRef Method, llvm::json::Value &&Params);


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the suggestions! After taking a closer look at boosting factors for 
other signals, I think I was being too conservative about boosting preferred 
scopes and penalizing non-preferred ones. I have tuned the parameters to make 
these more aggressive now according to your suggestion.




Comment at: clangd/Quality.cpp:262
+  Opts.UpCost = 1;
+  Opts.DownCost = 1;
+  Opts.AllowDownTraversalFromRoot = false;

sammccall wrote:
> I thought we concluded down cost was likely to be significantly higher than 
> up cost?
Sorry, forgot to make that change. Made UpCost=1, DownCost=2



Comment at: clangd/Quality.cpp:272
+else if (!S.empty() && Preferred.startswith(S))
+  Param.Cost = Opts.UpCost;
+else

sammccall wrote:
> This seems like an odd rule, what's the intuition?
>  - if the preferred scope is a::b::c:: then we're considering a::b:: and a:: 
> to be equal, rather than preferring the former
>  - you're defining the cost in terms of UpCost, but it's not clear why - is 
> this just being used as a scale?
>  - for the direct parent of the preferred scope, this is a no-op. So this 
> only does anything when the preferred scope is at least 3 deep
> 
> As a first approximation, `; // just rely on up-traversals` might be OK,
> otherwise, I guess you're trying to boost these compared to just relying on 
> up-traversals, so I'd expect this to look something like `Cost = UpCost * 
> (len(preferred) - len(S)) / 2`
> As a first approximation, ; // just rely on up-traversals might be OK,
This sounds good to me.



Comment at: clangd/Quality.cpp:276
+if (S == "")
+  Param.Cost += Opts.UpCost;
+Sources[Path.first] = std::move(Param);

sammccall wrote:
> Why is this +=?
> 
> Seems like there's three cases here:
>  - global is the preferred scope: I think the cost has to be 0 in this case, 
> no?
>  - global is another acceptable scope: some large fixed cost seems appropriate
>  - global is not listed: no source to worry about
> global is the preferred scope: I think the cost has to be 0 in this case, no?
I think we should still apply some penalty here because
  1. All projects can define symbols in the global scope. For example, if I 
work on a project without namespace, symbols from global namespaces are not 
necessarily relevant.
  2. A use pattern is that using-namespace is used in place of enclosing 
namespaces in implementation files. 

Otherwise, done.



Comment at: clangd/Quality.cpp:287
+  auto D = Distance->distance(scopeToPath(Scope).first);
+  return std::max(0.6, MaxBoost - D * 0.25);
+}

sammccall wrote:
> 0.6 seems too high for AnyScope matches, to me. And you're not distinguishing 
> anyScope matches from bad path matches.
> 
> I think the linear scale is too flat: if you're in clang::clangd::, then 
> clangd symbols will only get a 13% boost over clang ones.
> 
> Given the current UpCost == DownCost == 1, I'd consider something like
> ```
> if (D == FileDistance::Unreachable)
>   return 0.1;
> return max(0.5, MaxBoost * pow(0.6, D))
> ```
Done except for the scope for unreachable paths.

I think 0.1 is too much penalty and would probably make cross-namespace 
completions not useful. As cross-namespace completion is not enabled by 
default, it should be safe to give it a higher score (0.4?). 



Comment at: clangd/Quality.cpp:382
+// always in the accessible scope.
+Score *= SemaSaysInScope ? QueryScopeProximity::MaxBoost
+ : scopeBoost(*ScopeProximityMatch, SymbolScope);

sammccall wrote:
> hmm, if you have no scopes in the query I think you're boosting the sema 
> results and not the index ones. Intended?
Not intended. Changed to only boost scopes if there are query scopes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 169962.
ioeric marked 18 inline comments as done.
ioeric added a comment.

- Addressed review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/CodeComplete.cpp
  clangd/FileDistance.cpp
  clangd/FileDistance.h
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileDistanceTests.cpp
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -28,6 +28,7 @@
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -117,13 +118,14 @@
 
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) << "Decl in current file";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
+  << "Decl in current file";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6f) << "Decl from header";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 0.6f) << "Decl from header";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header_main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Current file and header";
 
   auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
@@ -146,10 +148,10 @@
 
   Relevance = {};
   Relevance.merge(constructShadowDeclCompletionResult("Bar"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
   Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
 
   Relevance = {};
@@ -210,9 +212,21 @@
   PoorNameMatch.NameMatch = 0.2f;
   EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
 
-  SymbolRelevanceSignals WithSemaProximity;
-  WithSemaProximity.SemaProximityScore = 0.2f;
-  EXPECT_GT(WithSemaProximity.evaluate(), Default.evaluate());
+  SymbolRelevanceSignals WithSemaFileProximity;
+  WithSemaFileProximity.SemaFileProximityScore = 0.2f;
+  EXPECT_GT(WithSemaFileProximity.evaluate(), Default.evaluate());
+
+  ScopeDistance ScopeProximity({"x::y::"});
+
+  SymbolRelevanceSignals WithSemaScopeProximity;
+  WithSemaScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithSemaScopeProximity.SemaSaysInScope = true;
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals WithIndexScopeProximity;
+  WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithIndexScopeProximity.SymbolScope = "x::";
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals IndexProximate;
   IndexProximate.SymbolURI = "unittest:/foo/bar.h";
@@ -242,6 +256,35 @@
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
+TEST(QualityTests, ScopeProximity) {
+  SymbolRelevanceSignals Relevance;
+  ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
+  Relevance.ScopeProximityMatch = &ScopeProximity;
+
+  Relevance.SymbolScope = "other::";
+  float NotMatched = Relevance.evaluate();
+
+  Relevance.SymbolScope = "";
+  float Global = Relevance.evaluate();
+  EXPECT_GT(Global, NotMatched);
+
+  Relevance.SymbolScope = "llvm::";
+  float NonParent = Relevance.evaluate();
+  EXPECT_GT(NonParent, Global);
+
+  Relevance.SymbolScope = "x::";
+  float GrandParent = Relevance.evaluate();
+  EXPECT_GT(GrandParent, Global);
+
+  Relevance.SymbolScope = "x::y::";
+  float Parent = Relevance.evaluate();
+  EXPECT_GT(Parent, GrandParent);
+
+  Relevance.SymbolScope = "x::y::z::";
+  float Enclosing = Relevance.evaluate();
+  EXPECT_GT(Enclosing, Parent);
+}
+
 TEST(QualityTests, SortText) {
   EXPECT_LT(sortText(std::numeric_limits::infinity()),
 sortText(1000.2f));
Index: unittests/clangd/FileDistanceTests.cpp
===
--- unittests/clangd/FileDistanceTests.cpp
+++ unittests/clangd/FileDistanceTests.cpp
@@ -109,6 +109,16 @@
   EXPECT_EQ(D.distance("/x"), FileDistance::Unreachable);
 }
 
+TEST(ScopeDistance, Smoke) {
+  ScopeDistance D({"x::y::z", "x::", "", "a::"});
+  EXPECT_EQ(D.distance("x::y::z::"), 0u);
+  EXPECT_GT(D.distance("x::y::"), D.distance("x::y::z::"));
+  EXPECT_GT(D.distance("x::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance("x::y::z::down::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance(""), D.distance("a::"));
+  EXPECT_GT(D.distance("

[PATCH] D53322: [clangd] Collect refs from headers.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 169963.
hokein marked 5 inline comments as done.
hokein added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53322

Files:
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -479,6 +479,17 @@
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _;
 }
 
+TEST_F(SymbolCollectorTest, RefsInHeaders) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+  class [[Foo]] {};
+  )");
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
 class W;
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -57,6 +57,11 @@
 /// The symbol ref kinds that will be collected.
 /// If not set, SymbolCollector will not collect refs.
 RefKind RefFilter = RefKind::Unknown;
+/// If set to true, SymbolCollector will collect all refs (from main file
+/// and included headers); otherwise, only refs from main file will be
+/// collected.
+/// This flag is only meaningful when RefFilter is set.
+bool RefsInHeaders = false;
 // Every symbol collected will be stamped with this origin.
 SymbolOrigin Origin = SymbolOrigin::Unknown;
 /// Collect macros.
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
@@ -354,7 +355,8 @@
 return true;
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
-  if (CollectRef && SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  if (CollectRef &&
+  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
@@ -474,26 +476,43 @@
   }
 
   const auto &SM = ASTCtx->getSourceManager();
-  auto* MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  llvm::DenseMap URICache;
+  auto GetURI = [&](clang::FileID FID) -> llvm::Optional {
+auto Found = URICache.find(FID);
+if (Found == URICache.end()) {
+  // Ignore cases where we can not find a corresponding file entry
+  // for the loc, thoses are not interesting, e.g. symbols formed
+  // via macro concatenation.
+  if (auto *FileEntry = SM.getFileEntryForID(FID)) {
+auto FileURI = toURI(SM, FileEntry->getName(), Opts);
+if (!FileURI) {
+  log("Failed to create URI for file: {0}\n", FileEntry);
+  FileURI = ""; // reset to empty as we also want to cache this case.
+}
+Found = URICache.insert({FID, *FileURI}).first;
+  }
+}
+return Found->second;
+  };
 
-  if (auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts)) {
-std::string MainURI = *MainFileURI;
+  if (auto MainFileURI = GetURI(SM.getMainFileID())) {
 for (const auto &It : DeclRefs) {
   if (auto ID = getSymbolID(It.first)) {
 for (const auto &LocAndRole : It.second) {
-  Ref R;
-  auto Range =
-  getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
-  R.Location.Start = Range.first;
-  R.Location.End = Range.second;
-  R.Location.FileURI = MainURI;
-  R.Kind = toRefKind(LocAndRole.second);
-  Refs.insert(*ID, R);
+  auto FileID = SM.getFileID(LocAndRole.first);
+  if (auto FileURI = GetURI(FileID)) {
+auto Range =
+getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
+Ref R;
+R.Location.Start = Range.first;
+R.Location.End = Range.second;
+R.Location.FileURI = *FileURI;
+R.Kind = toRefKind(LocAndRole.second);
+Refs.insert(*ID, R);
+  }
 }
   }
 }
-  } else {
-log("Failed to create URI for main file: {0}", MainFileEntry->getName());
   }
 
   ReferencedDecls.clear();
Index: clangd/index/IndexAction.h
==

[PATCH] D53322: [clangd] Collect refs from headers.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D53322#1266536, @sammccall wrote:

> (please do check there are no duplicates in the output)


We do deduplication when building the RefSlab. And double-checked with the 
output, no duplications there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53322



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53322: [clangd] Collect refs from headers.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 169967.
hokein added a comment.

minor cleanup.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53322

Files:
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -479,6 +479,17 @@
   EXPECT_THAT(Refs, Not(Contains(Pair(findSymbol(MainSymbols, "c").ID, _;
 }
 
+TEST_F(SymbolCollectorTest, RefsInHeaders) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+  class [[Foo]] {};
+  )");
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, References) {
   const std::string Header = R"(
 class W;
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -57,6 +57,11 @@
 /// The symbol ref kinds that will be collected.
 /// If not set, SymbolCollector will not collect refs.
 RefKind RefFilter = RefKind::Unknown;
+/// If set to true, SymbolCollector will collect all refs (from main file
+/// and included headers); otherwise, only refs from main file will be
+/// collected.
+/// This flag is only meaningful when RefFilter is set.
+bool RefsInHeaders = false;
 // Every symbol collected will be stamped with this origin.
 SymbolOrigin Origin = SymbolOrigin::Unknown;
 /// Collect macros.
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
@@ -354,7 +355,8 @@
 return true;
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
-  if (CollectRef && SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  if (CollectRef &&
+  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
@@ -474,26 +476,43 @@
   }
 
   const auto &SM = ASTCtx->getSourceManager();
-  auto* MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  llvm::DenseMap URICache;
+  auto GetURI = [&](FileID FID) -> llvm::Optional {
+auto Found = URICache.find(FID);
+if (Found == URICache.end()) {
+  // Ignore cases where we can not find a corresponding file entry
+  // for the loc, thoses are not interesting, e.g. symbols formed
+  // via macro concatenation.
+  if (auto *FileEntry = SM.getFileEntryForID(FID)) {
+auto FileURI = toURI(SM, FileEntry->getName(), Opts);
+if (!FileURI) {
+  log("Failed to create URI for file: {0}\n", FileEntry);
+  FileURI = ""; // reset to empty as we also want to cache this case.
+}
+Found = URICache.insert({FID, *FileURI}).first;
+  }
+}
+return Found->second;
+  };
 
-  if (auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts)) {
-std::string MainURI = *MainFileURI;
+  if (auto MainFileURI = GetURI(SM.getMainFileID())) {
 for (const auto &It : DeclRefs) {
   if (auto ID = getSymbolID(It.first)) {
 for (const auto &LocAndRole : It.second) {
-  Ref R;
-  auto Range =
-  getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
-  R.Location.Start = Range.first;
-  R.Location.End = Range.second;
-  R.Location.FileURI = MainURI;
-  R.Kind = toRefKind(LocAndRole.second);
-  Refs.insert(*ID, R);
+  auto FileID = SM.getFileID(LocAndRole.first);
+  if (auto FileURI = GetURI(FileID)) {
+auto Range =
+getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
+Ref R;
+R.Location.Start = Range.first;
+R.Location.End = Range.second;
+R.Location.FileURI = *FileURI;
+R.Kind = toRefKind(LocAndRole.second);
+Refs.insert(*ID, R);
+  }
 }
   }
 }
-  } else {
-log("Failed to create URI for main file: {0}", MainFileEntry->getName());
   }
 
   ReferencedDecls.clear();
Index: clangd/index/IndexAction.h
===
--- clangd/index/IndexAction.h
+++ c

[clang-tools-extra] r344678 - [clangd] Collect refs from headers.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Oct 17 01:38:36 2018
New Revision: 344678

URL: http://llvm.org/viewvc/llvm-project?rev=344678&view=rev
Log:
[clangd] Collect refs from headers.

Summary:
Add a flag to SymbolCollector to collect refs fdrom headers.

Note that we collect refs from headers in static index, and we don't do it for
dynamic index because of the preamble (we skip function body in preamble,
collecting it will result incomplete results).

Reviewers: sammccall

Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet, 
cfe-commits

Differential Revision: https://reviews.llvm.org/D53322

Modified:
clang-tools-extra/trunk/clangd/index/IndexAction.cpp
clang-tools-extra/trunk/clangd/index/IndexAction.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/IndexAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/IndexAction.cpp?rev=344678&r1=344677&r2=344678&view=diff
==
--- clang-tools-extra/trunk/clangd/index/IndexAction.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/IndexAction.cpp Wed Oct 17 01:38:36 
2018
@@ -66,8 +66,10 @@ createStaticIndexingAction(SymbolCollect
   Opts.CollectIncludePath = true;
   Opts.CountReferences = true;
   Opts.Origin = SymbolOrigin::Static;
-  if (RefsCallback != nullptr)
+  if (RefsCallback != nullptr) {
 Opts.RefFilter = RefKind::All;
+Opts.RefsInHeaders = true;
+  }
   auto Includes = llvm::make_unique();
   addSystemHeadersMapping(Includes.get());
   Opts.Includes = Includes.get();

Modified: clang-tools-extra/trunk/clangd/index/IndexAction.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/IndexAction.h?rev=344678&r1=344677&r2=344678&view=diff
==
--- clang-tools-extra/trunk/clangd/index/IndexAction.h (original)
+++ clang-tools-extra/trunk/clangd/index/IndexAction.h Wed Oct 17 01:38:36 2018
@@ -21,9 +21,8 @@ namespace clangd {
 // Only a subset of SymbolCollector::Options are respected:
 //   - include paths are always collected, and canonicalized appropriately
 //   - references are always counted
-//   - main-file refs are collected (if RefsCallback is non-null)
+//   - all references are collected (if RefsCallback is non-null)
 //   - the symbol origin is always Static
-// FIXME: refs from headers should also be collected.
 std::unique_ptr
 createStaticIndexingAction(SymbolCollector::Options Opts,
std::function SymbolsCallback,

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=344678&r1=344677&r2=344678&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Wed Oct 17 
01:38:36 2018
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
@@ -354,7 +355,8 @@ bool SymbolCollector::handleDeclOccurenc
 return true;
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
-  if (CollectRef && SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  if (CollectRef &&
+  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
@@ -474,26 +476,43 @@ void SymbolCollector::finish() {
   }
 
   const auto &SM = ASTCtx->getSourceManager();
-  auto* MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  llvm::DenseMap URICache;
+  auto GetURI = [&](FileID FID) -> llvm::Optional {
+auto Found = URICache.find(FID);
+if (Found == URICache.end()) {
+  // Ignore cases where we can not find a corresponding file entry
+  // for the loc, thoses are not interesting, e.g. symbols formed
+  // via macro concatenation.
+  if (auto *FileEntry = SM.getFileEntryForID(FID)) {
+auto FileURI = toURI(SM, FileEntry->getName(), Opts);
+if (!FileURI) {
+  log("Failed to create URI for file: {0}\n", FileEntry);
+  FileURI = ""; // reset to empty as we also want to cache this case.
+}
+Found = URICache.insert({FID, *FileURI}).first;
+  }
+}
+return Found->second;
+  };
 
-  if (auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts)) {
-std::string MainURI = *MainFileURI;
+  if (auto MainFileURI

[PATCH] D53322: [clangd] Collect refs from headers.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344678: [clangd] Collect refs from headers. (authored by 
hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53322?vs=169967&id=169968#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53322

Files:
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -57,6 +57,11 @@
 /// The symbol ref kinds that will be collected.
 /// If not set, SymbolCollector will not collect refs.
 RefKind RefFilter = RefKind::Unknown;
+/// If set to true, SymbolCollector will collect all refs (from main file
+/// and included headers); otherwise, only refs from main file will be
+/// collected.
+/// This flag is only meaningful when RefFilter is set.
+bool RefsInHeaders = false;
 // Every symbol collected will be stamped with this origin.
 SymbolOrigin Origin = SymbolOrigin::Unknown;
 /// Collect macros.
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -19,6 +19,7 @@
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
@@ -354,7 +355,8 @@
 return true;
   if (!shouldCollectSymbol(*ND, *ASTCtx, Opts))
 return true;
-  if (CollectRef && SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  if (CollectRef &&
+  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
 DeclRefs[ND].emplace_back(SpellingLoc, Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
@@ -474,26 +476,43 @@
   }
 
   const auto &SM = ASTCtx->getSourceManager();
-  auto* MainFileEntry = SM.getFileEntryForID(SM.getMainFileID());
+  llvm::DenseMap URICache;
+  auto GetURI = [&](FileID FID) -> llvm::Optional {
+auto Found = URICache.find(FID);
+if (Found == URICache.end()) {
+  // Ignore cases where we can not find a corresponding file entry
+  // for the loc, thoses are not interesting, e.g. symbols formed
+  // via macro concatenation.
+  if (auto *FileEntry = SM.getFileEntryForID(FID)) {
+auto FileURI = toURI(SM, FileEntry->getName(), Opts);
+if (!FileURI) {
+  log("Failed to create URI for file: {0}\n", FileEntry);
+  FileURI = ""; // reset to empty as we also want to cache this case.
+}
+Found = URICache.insert({FID, *FileURI}).first;
+  }
+}
+return Found->second;
+  };
 
-  if (auto MainFileURI = toURI(SM, MainFileEntry->getName(), Opts)) {
-std::string MainURI = *MainFileURI;
+  if (auto MainFileURI = GetURI(SM.getMainFileID())) {
 for (const auto &It : DeclRefs) {
   if (auto ID = getSymbolID(It.first)) {
 for (const auto &LocAndRole : It.second) {
-  Ref R;
-  auto Range =
-  getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
-  R.Location.Start = Range.first;
-  R.Location.End = Range.second;
-  R.Location.FileURI = MainURI;
-  R.Kind = toRefKind(LocAndRole.second);
-  Refs.insert(*ID, R);
+  auto FileID = SM.getFileID(LocAndRole.first);
+  if (auto FileURI = GetURI(FileID)) {
+auto Range =
+getTokenRange(LocAndRole.first, SM, ASTCtx->getLangOpts());
+Ref R;
+R.Location.Start = Range.first;
+R.Location.End = Range.second;
+R.Location.FileURI = *FileURI;
+R.Kind = toRefKind(LocAndRole.second);
+Refs.insert(*ID, R);
+  }
 }
   }
 }
-  } else {
-log("Failed to create URI for main file: {0}", MainFileEntry->getName());
   }
 
   ReferencedDecls.clear();
Index: clangd/index/IndexAction.h
===
--- clangd/index/IndexAction.h
+++ clangd/index/IndexAction.h
@@ -21,9 +21,8 @@
 // Only a subset of SymbolCollector::Options are respected:
 //   - include paths are always collected, and canonicalized appropriately
 //   - references are always counted
-//   - main-file refs are collected (if RefsCallback is non-null)
+//   - all references are collected (if RefsCallback is non-null)
 //   - the symbol origin is always Static
-// FIXME: refs from headers should also be collected.
 std::unique_ptr
 createStaticIndexingAction(SymbolCollector::Options Opts,
std::function SymbolsCallback,
Index: 

[clang-tools-extra] r344679 - [clangd] Print numbers of symbols and refs as well when loading the

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Oct 17 01:48:04 2018
New Revision: 344679

URL: http://llvm.org/viewvc/llvm-project?rev=344679&view=rev
Log:
[clangd] Print numbers of symbols and refs as well when loading the
index.

Modified:
clang-tools-extra/trunk/clangd/index/Serialization.cpp

Modified: clang-tools-extra/trunk/clangd/index/Serialization.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Serialization.cpp?rev=344679&r1=344678&r2=344679&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Serialization.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Serialization.cpp Wed Oct 17 01:48:04 
2018
@@ -496,13 +496,18 @@ std::unique_ptr loadIndex(l
 }
   }
 
+  size_t SymSize = Symbols.size();
+  size_t RefSize = Refs.size();
   trace::Span Tracer("BuildIndex");
   auto Index =
   UseDex ? dex::Dex::build(std::move(Symbols), std::move(Refs), URISchemes)
  : MemIndex::build(std::move(Symbols), std::move(Refs));
-  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+  vlog("Loaded {0} from {1} with estimated memory usage {2} bytes\n"
+   "  - number of symbos: {3}\n"
+   "  - number of refs: {4}\n",
UseDex ? "Dex" : "MemIndex", SymbolFilename,
-   Index->estimateMemoryUsage());
+   Index->estimateMemoryUsage(),
+   SymSize, RefSize);
   return Index;
 }
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r344680 - [clangd] Fix buildbot failure.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Oct 17 01:54:48 2018
New Revision: 344680

URL: http://llvm.org/viewvc/llvm-project?rev=344680&view=rev
Log:
[clangd] Fix buildbot failure.

Modified:
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=344680&r1=344679&r2=344680&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Wed Oct 17 
01:54:48 2018
@@ -480,9 +480,6 @@ void SymbolCollector::finish() {
   auto GetURI = [&](FileID FID) -> llvm::Optional {
 auto Found = URICache.find(FID);
 if (Found == URICache.end()) {
-  // Ignore cases where we can not find a corresponding file entry
-  // for the loc, thoses are not interesting, e.g. symbols formed
-  // via macro concatenation.
   if (auto *FileEntry = SM.getFileEntryForID(FID)) {
 auto FileURI = toURI(SM, FileEntry->getName(), Opts);
 if (!FileURI) {
@@ -490,6 +487,11 @@ void SymbolCollector::finish() {
   FileURI = ""; // reset to empty as we also want to cache this case.
 }
 Found = URICache.insert({FID, *FileURI}).first;
+  } else {
+// Ignore cases where we can not find a corresponding file entry
+// for the loc, thoses are not interesting, e.g. symbols formed
+// via macro concatenation.
+return llvm::None;
   }
 }
 return Found->second;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r344650 - [clang-doc] Add unit tests for serialization

[Mikael Holmén via cfe-commits]

Hi Julie,

clang 3.6.0 complains on this commit:

/usr/bin/clang++  -march=corei7  -DGTEST_HAS_RTTI=0 
-DGTEST_HAS_TR1_TUPLE=0 -DGTEST_LANG_CXX11=1 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/clang/tools/extra/unittests/clang-doc 
-I../tools/clang/tools/extra/unittests/clang-doc 
-I../tools/clang/include -Itools/clang/include -I/usr/include/libxml2 
-Iinclude -I../include -I../tools/clang/tools/extra/clang-doc 
-I../utils/unittest/googletest/include 
-I../utils/unittest/googlemock/include 
-I/proj/flexasic/app/valgrind/3.11.0/include  -fPIC 
-fvisibility-inlines-hidden -Werror -Werror=date-time -std=c++11 -Wall 
-Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wmissing-field-initializers -pedantic -Wno-long-long 
-Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -ffunction-sections 
-fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types 
-O3-UNDEBUG  -Wno-variadic-macros 
-Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -MMD 
-MT 
tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
 
-MF 
tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o.d
 
-o 
tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
 
-c ../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp
In file included from 
../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp:12:
../tools/clang/tools/extra/unittests/clang-doc/ClangDocTest.h:25:14: 
error: suggest braces around initialization of subobject 
[-Werror,-Wmissing-braces]
 SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
  ^~
  { }
1 error generated.

Regards,
Mikael

On 10/17/2018 01:06 AM, Julie Hockett via cfe-commits wrote:
> Author: juliehockett
> Date: Tue Oct 16 16:06:42 2018
> New Revision: 344650
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=344650&view=rev
> Log:
> [clang-doc] Add unit tests for serialization
> 
> Adds unit tests for the Serialize library.
> 
> This is part of a move to convert clang-doc's tests to a more
> maintainable unit test framework, with a smaller number of integration
> tests to maintain and more granular failure feedback.
> 
> Differential Revision: https://reviews.llvm.org/D53081
> 
> Added:
>  clang-tools-extra/trunk/unittests/clang-doc/
>  clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
>  clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp
>  clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
>  clang-tools-extra/trunk/unittests/clang-doc/SerializeTest.cpp
> Modified:
>  clang-tools-extra/trunk/unittests/CMakeLists.txt
> 
> Modified: clang-tools-extra/trunk/unittests/CMakeLists.txt
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/CMakeLists.txt?rev=344650&r1=344649&r2=344650&view=diff
> ==
> --- clang-tools-extra/trunk/unittests/CMakeLists.txt (original)
> +++ clang-tools-extra/trunk/unittests/CMakeLists.txt Tue Oct 16 16:06:42 2018
> @@ -16,6 +16,7 @@ endif()
>   
>   add_subdirectory(change-namespace)
>   add_subdirectory(clang-apply-replacements)
> +add_subdirectory(clang-doc)
>   add_subdirectory(clang-move)
>   add_subdirectory(clang-query)
>   add_subdirectory(clang-tidy)
> 
> Added: clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt?rev=344650&view=auto
> ==
> --- clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt (added)
> +++ clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt Tue Oct 16 
> 16:06:42 2018
> @@ -0,0 +1,29 @@
> +set(LLVM_LINK_COMPONENTS
> +  support
> +  BitReader
> +  BitWriter
> +  )
> +
> +get_filename_component(CLANG_DOC_SOURCE_DIR
> +  ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-doc REALPATH)
> +include_directories(
> +  ${CLANG_DOC_SOURCE_DIR}
> +  )
> +
> +add_extra_unittest(ClangDocTests
> +  ClangDocTest.cpp
> +  SerializeTest.cpp
> +  )
> +
> +target_link_libraries(ClangDocTests
> +  PRIVATE
> +  clangAST
> +  clangASTMatchers
> +  clangBasic
> +  clangDoc
> +  clangFormat
> +  clangFrontend
> +  clangRewrite
> +  clangTooling
> +  clangToolingCore
> +  )
> 
> Added: clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp?rev=344650&view=auto
> ==
> --- clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp (added)
>

[PATCH] D53024: [analyzer][www] Add more open projects

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added inline comments.



Comment at: www/analyzer/open_projects.html:198
+  or using a dataflow framework.
+  (Difficulty: Hard)
+

Probably it is worth mentioning here, that there is a macro language already 
for describing summaries of standard library functions in 
StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp. 

This macro language could be refactored to a separate header file so it could 
be used in other checkers too. Could also be extended for C++. 

Another useful addition would be to enable users to describe these summaries in 
run-time (in YAML files for example) to be able to model their own proprietary 
library functions. 

Then as a next step we could introduce a flow sensitive analysis to generate 
such summaries automatically. Which is a hard problem indeed, the others above 
should not be too difficult


Repository:
  rC Clang

https://reviews.llvm.org/D53024



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome! Just nits.




Comment at: clangd/CodeComplete.cpp:1224
   std::vector QueryScopes; // Initialized once Sema runs.
+  // Initialized once QueryScopes is initialized.
+  llvm::Optional ScopeProximity;

, if there are scopes?



Comment at: clangd/FileDistance.cpp:206
+// The global namespace is not 'near' its children.
+Param.MaxUpTraversals = std::max(Path.second - 1, 0);
+Sources[Path.first] = std::move(Param);

I think this sets MaxUpTraversals to -1 for the empty scope.
*probably* harmless in the end, but I think an explicit `if !S.empty()` might 
be clearer



Comment at: clangd/FileDistance.h:125
+private:
+  std::unique_ptr Distance;
+};

nit: It seems slightly odd to make this a unique_ptr just to make the 
constructor simpler - could also just define a helper function in the CC file 
that the constructor could call: `ScopeDistance::ScopeDistance(ArrayRef 
Scopes) : Distance(createScopeFileDistance(Scopes))`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52400: Improve -Wshadow warnings with enumerators

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

In https://reviews.llvm.org/D52400#1266341, @aaron.ballman wrote:

> In https://reviews.llvm.org/D52400#1266307, @sberg wrote:
>
> >
>


[...]

> Then again, this is a case where you don't get any error but you do get a 
> silent behavioral ambiguity without the current enumerator shadow diagnostic:
> 
>   struct S1;
>   struct S2;
>   struct S3 {
> void S1();
> enum { S2 };
>   
> void f(decltype(S2) s);
>   };
> 
> 
> So there are cases where this behavior can be somewhat useful.

but decltype(S2) is a syntax error when S2 names a struct type, no?

>> (ran into such a new -Wshadow while compiling LibreOffice)
> 
> Was it a frequent/annoying occurrence?

there was less than 20 cases overall. about half were "good" warnings about 
clashing enumerators from different non-scoped enums. the others were 
"unhelpful" ones about clashes with class names, two of them in stable 
interface code that cant be changed (so would need ugly #pragma clang warning 
decorations), one of them even about entities in unrelated include files, so 
whether a warning is emitted depends on the order in which the files happen to 
get included in a TU

(and in any case, "declaration shadows a variable" sounds wrong when the 
shadowed entity is a class type. thats why I thought something is not quite 
right with this new code yet)


https://reviews.llvm.org/D52400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov updated this revision to Diff 169978.
AlexeySachkov added a comment.

Removed unnecessary empty line from test


https://reviews.llvm.org/D53200

Files:
  lib/Serialization/ASTWriter.cpp
  test/Headers/opencl-pragma-extension-begin.cl
  test/Headers/opencl-pragma-extension-begin.h


Index: test/Headers/opencl-pragma-extension-begin.h
===
--- /dev/null
+++ test/Headers/opencl-pragma-extension-begin.h
@@ -0,0 +1,3 @@
+#pragma OPENCL EXTENSION cl_my_ext : begin
+void cl_my_ext_foo();
+#pragma OPENCL EXTENSION cl_my_ext : end
Index: test/Headers/opencl-pragma-extension-begin.cl
===
--- /dev/null
+++ test/Headers/opencl-pragma-extension-begin.cl
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: %clang_cc1 -cl-std=CL1.2 -include %S/opencl-pragma-extension-begin.h 
-triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t %s 2>&1
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: %clang_cc1 -cl-std=CL2.0 -include %S/opencl-pragma-extension-begin.h 
-triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules 
-fimplicit-module-maps -fmodules-cache-path=%t %s 2>&1
+
+void __kernel test(__global int *data) {
+  *data = 10;
+}
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -5014,13 +5014,16 @@
   WriteFPPragmaOptions(SemaRef.getFPOptions());
   WriteOpenCLExtensions(SemaRef);
   WriteOpenCLExtensionTypes(SemaRef);
-  WriteOpenCLExtensionDecls(SemaRef);
   WriteCUDAPragmas(SemaRef);
 
   // If we're emitting a module, write out the submodule information.
   if (WritingModule)
 WriteSubmodules(WritingModule);
 
+  // We need to have information about submodules to correctly deserialize
+  // decls from OpenCLExtensionDecls block
+  WriteOpenCLExtensionDecls(SemaRef);
+
   Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
 
   // Write the record containing external, unnamed definitions.


Index: test/Headers/opencl-pragma-extension-begin.h
===
--- /dev/null
+++ test/Headers/opencl-pragma-extension-begin.h
@@ -0,0 +1,3 @@
+#pragma OPENCL EXTENSION cl_my_ext : begin
+void cl_my_ext_foo();
+#pragma OPENCL EXTENSION cl_my_ext : end
Index: test/Headers/opencl-pragma-extension-begin.cl
===
--- /dev/null
+++ test/Headers/opencl-pragma-extension-begin.cl
@@ -0,0 +1,13 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: %clang_cc1 -cl-std=CL1.2 -include %S/opencl-pragma-extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %s 2>&1
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: %clang_cc1 -cl-std=CL2.0 -include %S/opencl-pragma-extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %s 2>&1
+
+void __kernel test(__global int *data) {
+  *data = 10;
+}
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -5014,13 +5014,16 @@
   WriteFPPragmaOptions(SemaRef.getFPOptions());
   WriteOpenCLExtensions(SemaRef);
   WriteOpenCLExtensionTypes(SemaRef);
-  WriteOpenCLExtensionDecls(SemaRef);
   WriteCUDAPragmas(SemaRef);
 
   // If we're emitting a module, write out the submodule information.
   if (WritingModule)
 WriteSubmodules(WritingModule);
 
+  // We need to have information about submodules to correctly deserialize
+  // decls from OpenCLExtensionDecls block
+  WriteOpenCLExtensionDecls(SemaRef);
+
   Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
 
   // Write the record containing external, unnamed definitions.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51633: [ASTImporter] Added error handling for AST import.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

In the current version all clang tests do pass and most of the LLDB tests. Some 
LLDB tests do not work on my machine, the problems exist on master too.
This code is still a temporary version (possible to commit now or wait until 
every change is made?). Later the `Import` functions should return `Expected` 
and the remaining functions in ASTImporter.cpp should be updated. For 
compatibility with old code in LLDB the `Import` functions can not be changed 
without LLDB code changes so a `Import_New` will be created instead. Code in 
clang will only use the `Import_New`. When LLDB was updated, `Import_New` 
should be renamed to `Import`.


Repository:
  rC Clang

https://reviews.llvm.org/D51633



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53200: [OpenCL] Fix serialization of OpenCLExtensionDecls

[Alexey Sachkov via Phabricator via cfe-commits]

AlexeySachkov added a comment.

@yaxunl could you please review this patch?

It fixes a bug in a feature implemented by you in:

  commit c6fb598a301143e9d21156a012cc6ef669ff0188   
 
  Author: Yaxun Liu  
 
  Date:   Sun Dec 18 05:18:55 2016 +
 

 
  Recommit r289979 [OpenCL] Allow disabling types and declarations 
associated with extensions

 
  Fixed undefined behavior due to cast integer to bool in initializer list. 
 

 

 
  git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290056 
91177308-0d34-0410-b5e6-96231b3b80d8 


https://reviews.llvm.org/D53200



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.

This would buy us more memory. Using a 32-bits integer is enough for
most human-readable source code (up to 4M lines and 4K columns).

Previsouly, we used 8 bytes for a position, now 4 bytes, it would save
us 8 bytes for each Ref and each Symbol instance.

For LLVM-project binary index file, we save ~13% memory.

| Before | After |
| 412MB  | 355MB |


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363

Files:
  clangd/index/Index.h
  clangd/index/YAMLSerialization.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/SerializationTests.cpp

Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -33,12 +33,8 @@
   Lang:Cpp
 CanonicalDeclaration:
   FileURI:file:///path/foo.h
-  Start:
-Line: 1
-Column: 0
-  End:
-Line: 1
-Column: 1
+  Start: 4096 # Line 1, Column 0
+  End: 4097 # Line 1, Column 1
 Origin:4
 Flags:1
 Documentation:'Foo doc'
@@ -59,12 +55,8 @@
   Lang:Cpp
 CanonicalDeclaration:
   FileURI:file:///path/bar.h
-  Start:
-Line: 1
-Column: 0
-  End:
-Line: 1
-Column: 1
+  Start: 4096 # Line 1, Column 0
+  End: 4097 # Line 1, Column 1
 Flags:2
 Signature:'-sig'
 CompletionSnippetSuffix:'-snippet'
@@ -75,12 +67,8 @@
   - Kind: 4
 Location:
   FileURI:file:///path/foo.cc
-  Start:
-Line: 5
-Column: 3
-  End:
-Line: 5
-Column: 8
+  Start: 20483 # Line 5, Column 3
+  End: 20485 # Line5, Column 8
 )";
 
 MATCHER_P(ID, I, "") { return arg.ID == cantFail(SymbolID::fromStr(I)); }
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -32,10 +32,10 @@
 using testing::UnorderedElementsAre;
 
 MATCHER_P(RefRange, Range, "") {
-  return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
-  arg.Location.End.Line, arg.Location.End.Column) ==
- std::tie(Range.start.line, Range.start.character, Range.end.line,
-  Range.end.character);
+  return arg.Location.Start.Line == Range.start.line &&
+ arg.Location.Start.Column == Range.start.character &&
+ arg.Location.End.Line == Range.end.line &&
+ arg.Location.End.Column == Range.end.character;
 }
 MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
 MATCHER_P(DeclURI, U, "") { return arg.CanonicalDeclaration.FileURI == U; }
Index: clangd/index/YAMLSerialization.cpp
===
--- clangd/index/YAMLSerialization.cpp
+++ clangd/index/YAMLSerialization.cpp
@@ -94,18 +94,37 @@
   uint8_t Origin = 0;
 };
 
-template <> struct MappingTraits {
-  static void mapping(IO &IO, SymbolLocation::Position &Value) {
-IO.mapRequired("Line", Value.Line);
-IO.mapRequired("Column", Value.Column);
+struct NormalizedPosition {
+  using Position = clang::clangd::SymbolLocation::Position;
+  NormalizedPosition(IO &) {}
+  NormalizedPosition(IO &, const Position &Pos) {
+static_assert(
+sizeof(Position) == sizeof(uint32_t),
+"SymbolLocation::Position structure can not fit into a uint32_t.");
+value = (Pos.Line << Position::ColumnBits) + Pos.Column;
   }
+
+  Position denormalize(IO &) {
+Position Pos;
+Pos.Line = value >> Position::ColumnBits;
+Pos.Column = value & ((1 << Position::ColumnBits) - 1);
+return Pos;
+  }
+
+  // Encode the SymbolLocation::Position:
+  // |  Line  | Column |
+  uint32_t value;
 };
 
 template <> struct MappingTraits {
   static void mapping(IO &IO, SymbolLocation &Value) {
 IO.mapRequired("FileURI", Value.FileURI);
-IO.mapRequired("Start", Value.Start);
-IO.mapRequired("End", Value.End);
+MappingNormalization NStart(
+IO, Value.Start);
+IO.mapRequired("Start", NStart->value);
+MappingNormalization NEnd(
+IO, Value.End);
+IO.mapRequired("End", NEnd->value);
   }
 };
 
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -33,10 +33,17 @@
 struct SymbolLocation {
   // Specify a position (Line, Column) of symbol. Using Line/Column allows us to
   // build LSP responses without reading the file content.
+  //
+  // Position is encoded as a 32-bits integer like below:
+  // |<--- 20 bits --->|<-12 bits->|
+  // |<---  Line   --->|<- Column->|
   struct Position {
-uint32_t Line = 0; // 0-based
+static constexpr int LineBits = 20;
+static constexpr int ColumnBits = 12;
+
+uint32_t Line : LineBits; // 0-based
 /

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 169982.
ioeric marked an inline comment as done.
ioeric added a comment.

- address comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/CodeComplete.cpp
  clangd/FileDistance.cpp
  clangd/FileDistance.h
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileDistanceTests.cpp
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -28,6 +28,7 @@
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -117,13 +118,14 @@
 
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) << "Decl in current file";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
+  << "Decl in current file";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6f) << "Decl from header";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 0.6f) << "Decl from header";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header_main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Current file and header";
 
   auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
@@ -146,10 +148,10 @@
 
   Relevance = {};
   Relevance.merge(constructShadowDeclCompletionResult("Bar"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
   Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
 
   Relevance = {};
@@ -210,9 +212,21 @@
   PoorNameMatch.NameMatch = 0.2f;
   EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
 
-  SymbolRelevanceSignals WithSemaProximity;
-  WithSemaProximity.SemaProximityScore = 0.2f;
-  EXPECT_GT(WithSemaProximity.evaluate(), Default.evaluate());
+  SymbolRelevanceSignals WithSemaFileProximity;
+  WithSemaFileProximity.SemaFileProximityScore = 0.2f;
+  EXPECT_GT(WithSemaFileProximity.evaluate(), Default.evaluate());
+
+  ScopeDistance ScopeProximity({"x::y::"});
+
+  SymbolRelevanceSignals WithSemaScopeProximity;
+  WithSemaScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithSemaScopeProximity.SemaSaysInScope = true;
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals WithIndexScopeProximity;
+  WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithIndexScopeProximity.SymbolScope = "x::";
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals IndexProximate;
   IndexProximate.SymbolURI = "unittest:/foo/bar.h";
@@ -242,6 +256,35 @@
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
+TEST(QualityTests, ScopeProximity) {
+  SymbolRelevanceSignals Relevance;
+  ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
+  Relevance.ScopeProximityMatch = &ScopeProximity;
+
+  Relevance.SymbolScope = "other::";
+  float NotMatched = Relevance.evaluate();
+
+  Relevance.SymbolScope = "";
+  float Global = Relevance.evaluate();
+  EXPECT_GT(Global, NotMatched);
+
+  Relevance.SymbolScope = "llvm::";
+  float NonParent = Relevance.evaluate();
+  EXPECT_GT(NonParent, Global);
+
+  Relevance.SymbolScope = "x::";
+  float GrandParent = Relevance.evaluate();
+  EXPECT_GT(GrandParent, Global);
+
+  Relevance.SymbolScope = "x::y::";
+  float Parent = Relevance.evaluate();
+  EXPECT_GT(Parent, GrandParent);
+
+  Relevance.SymbolScope = "x::y::z::";
+  float Enclosing = Relevance.evaluate();
+  EXPECT_GT(Enclosing, Parent);
+}
+
 TEST(QualityTests, SortText) {
   EXPECT_LT(sortText(std::numeric_limits::infinity()),
 sortText(1000.2f));
Index: unittests/clangd/FileDistanceTests.cpp
===
--- unittests/clangd/FileDistanceTests.cpp
+++ unittests/clangd/FileDistanceTests.cpp
@@ -95,6 +95,30 @@
   EXPECT_EQ(D.distance("/a/b/z"), 2u);
 }
 
+TEST(FileDistance, DisallowDownTraversalsFromRoot) {
+  FileDistanceOptions Opts;
+  Opts.UpCost = Opts.DownCost = 1;
+  Opts.AllowDownTraversalFromRoot = false;
+  SourceParams CostLots;
+  CostLots.Cost = 100;
+
+  FileDistance D({{"/", SourceParams()}, {"/a/b/c", CostLots}}, Opts);
+  EXPECT_EQ(D.distance("/"), 0u);
+  EXPECT_EQ(D.distance("/a"), 102u);
+  EXPECT_EQ(D.distance("/a/b"), 101u);
+  EXPECT_EQ(

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 169983.
ioeric added a comment.

- rebase


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53131

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/CodeComplete.cpp
  clangd/FileDistance.cpp
  clangd/FileDistance.h
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileDistanceTests.cpp
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -28,6 +28,7 @@
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -117,13 +118,14 @@
 
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) << "Decl in current file";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
+  << "Decl in current file";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6f) << "Decl from header";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 0.6f) << "Decl from header";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header_main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Current file and header";
 
   auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
@@ -146,10 +148,10 @@
 
   Relevance = {};
   Relevance.merge(constructShadowDeclCompletionResult("Bar"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
   Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
 
   Relevance = {};
@@ -210,9 +212,21 @@
   PoorNameMatch.NameMatch = 0.2f;
   EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
 
-  SymbolRelevanceSignals WithSemaProximity;
-  WithSemaProximity.SemaProximityScore = 0.2f;
-  EXPECT_GT(WithSemaProximity.evaluate(), Default.evaluate());
+  SymbolRelevanceSignals WithSemaFileProximity;
+  WithSemaFileProximity.SemaFileProximityScore = 0.2f;
+  EXPECT_GT(WithSemaFileProximity.evaluate(), Default.evaluate());
+
+  ScopeDistance ScopeProximity({"x::y::"});
+
+  SymbolRelevanceSignals WithSemaScopeProximity;
+  WithSemaScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithSemaScopeProximity.SemaSaysInScope = true;
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals WithIndexScopeProximity;
+  WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithIndexScopeProximity.SymbolScope = "x::";
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals IndexProximate;
   IndexProximate.SymbolURI = "unittest:/foo/bar.h";
@@ -242,6 +256,35 @@
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
+TEST(QualityTests, ScopeProximity) {
+  SymbolRelevanceSignals Relevance;
+  ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
+  Relevance.ScopeProximityMatch = &ScopeProximity;
+
+  Relevance.SymbolScope = "other::";
+  float NotMatched = Relevance.evaluate();
+
+  Relevance.SymbolScope = "";
+  float Global = Relevance.evaluate();
+  EXPECT_GT(Global, NotMatched);
+
+  Relevance.SymbolScope = "llvm::";
+  float NonParent = Relevance.evaluate();
+  EXPECT_GT(NonParent, Global);
+
+  Relevance.SymbolScope = "x::";
+  float GrandParent = Relevance.evaluate();
+  EXPECT_GT(GrandParent, Global);
+
+  Relevance.SymbolScope = "x::y::";
+  float Parent = Relevance.evaluate();
+  EXPECT_GT(Parent, GrandParent);
+
+  Relevance.SymbolScope = "x::y::z::";
+  float Enclosing = Relevance.evaluate();
+  EXPECT_GT(Enclosing, Parent);
+}
+
 TEST(QualityTests, SortText) {
   EXPECT_LT(sortText(std::numeric_limits::infinity()),
 sortText(1000.2f));
Index: unittests/clangd/FileDistanceTests.cpp
===
--- unittests/clangd/FileDistanceTests.cpp
+++ unittests/clangd/FileDistanceTests.cpp
@@ -109,6 +109,16 @@
   EXPECT_EQ(D.distance("/x"), FileDistance::Unreachable);
 }
 
+TEST(ScopeDistance, Smoke) {
+  ScopeDistance D({"x::y::z", "x::", "", "a::"});
+  EXPECT_EQ(D.distance("x::y::z::"), 0u);
+  EXPECT_GT(D.distance("x::y::"), D.distance("x::y::z::"));
+  EXPECT_GT(D.distance("x::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance("x::y::z::down::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance(""), D.distance("a::"));
+  EXPECT_GT(D.distance("x::"), D.distance("a::"));
+}
+
 } // namespace
 } // namespac

[clang-tools-extra] r344688 - [clangd] Support scope proximity in code completion.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Wed Oct 17 04:19:02 2018
New Revision: 344688

URL: http://llvm.org/viewvc/llvm-project?rev=344688&view=rev
Log:
[clangd] Support scope proximity in code completion.

Summary:
This should make all-scope completion more usable. Scope proximity for
indexes will be added in followup patch.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D53131

Modified:
clang-tools-extra/trunk/clangd/AST.cpp
clang-tools-extra/trunk/clangd/AST.h
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/FileDistance.cpp
clang-tools-extra/trunk/clangd/FileDistance.h
clang-tools-extra/trunk/clangd/Quality.cpp
clang-tools-extra/trunk/clangd/Quality.h
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp

Modified: clang-tools-extra/trunk/clangd/AST.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.cpp?rev=344688&r1=344687&r2=344688&view=diff
==
--- clang-tools-extra/trunk/clangd/AST.cpp (original)
+++ clang-tools-extra/trunk/clangd/AST.cpp Wed Oct 17 04:19:02 2018
@@ -54,6 +54,14 @@ std::string printQualifiedName(const Nam
   return QName;
 }
 
+std::string printNamespaceScope(const DeclContext &DC) {
+  for (const auto *Ctx = &DC; Ctx != nullptr; Ctx = Ctx->getParent())
+if (const auto *NS = dyn_cast(Ctx))
+  if (!NS->isAnonymousNamespace() && !NS->isInlineNamespace())
+return printQualifiedName(*NS) + "::";
+  return "";
+}
+
 llvm::Optional getSymbolID(const Decl *D) {
   llvm::SmallString<128> USR;
   if (index::generateUSRForDecl(D, USR))

Modified: clang-tools-extra/trunk/clangd/AST.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/AST.h?rev=344688&r1=344687&r2=344688&view=diff
==
--- clang-tools-extra/trunk/clangd/AST.h (original)
+++ clang-tools-extra/trunk/clangd/AST.h Wed Oct 17 04:19:02 2018
@@ -34,6 +34,9 @@ SourceLocation findNameLoc(const clang::
 /// like inline namespaces.
 std::string printQualifiedName(const NamedDecl &ND);
 
+/// Returns the first enclosing namespace scope starting from \p DC.
+std::string printNamespaceScope(const DeclContext &DC);
+
 /// Gets the symbol ID for a declaration, if possible.
 llvm::Optional getSymbolID(const Decl *D);
 

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=344688&r1=344687&r2=344688&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Wed Oct 17 04:19:02 2018
@@ -34,6 +34,8 @@
 #include "Trace.h"
 #include "URI.h"
 #include "index/Index.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
@@ -44,6 +46,7 @@
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
@@ -549,9 +552,10 @@ struct SpecifiedScope {
 };
 
 // Get all scopes that will be queried in indexes and whether symbols from
-// any scope is allowed.
+// any scope is allowed. The first scope in the list is the preferred scope
+// (e.g. enclosing namespace).
 std::pair, bool>
-getQueryScopes(CodeCompletionContext &CCContext, const SourceManager &SM,
+getQueryScopes(CodeCompletionContext &CCContext, const Sema &CCSema,
const CodeCompleteOptions &Opts) {
   auto GetAllAccessibleScopes = [](CodeCompletionContext &CCContext) {
 SpecifiedScope Info;
@@ -559,7 +563,7 @@ getQueryScopes(CodeCompletionContext &CC
   if (isa(Context))
 Info.AccessibleScopes.push_back(""); // global namespace
   else if (const auto *NS = dyn_cast(Context))
-Info.AccessibleScopes.push_back(NS->getQualifiedNameAsString() + "::");
+Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
 }
 return Info;
   };
@@ -568,12 +572,13 @@ getQueryScopes(CodeCompletionContext &CC
 
   // Unqualified completion (e.g. "vec^").
   if (!SS) {
-// FIXME: Once we can insert namespace qualifiers and use the in-scope
-//namespaces for scoring, search in all namespaces.
-// FIXME: Capture scopes and use for scoring, for example,
-//"using namespace std; namespace foo {v^}" =>
-//foo::value > std::vector > boost::variant
-auto Sco

[PATCH] D53131: [clangd] Support scope proximity in code completion.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344688: [clangd] Support scope proximity in code completion. 
(authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D53131

Files:
  clang-tools-extra/trunk/clangd/AST.cpp
  clang-tools-extra/trunk/clangd/AST.h
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/FileDistance.cpp
  clang-tools-extra/trunk/clangd/FileDistance.h
  clang-tools-extra/trunk/clangd/Quality.cpp
  clang-tools-extra/trunk/clangd/Quality.h
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
  clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FileDistanceTests.cpp
@@ -109,6 +109,16 @@
   EXPECT_EQ(D.distance("/x"), FileDistance::Unreachable);
 }
 
+TEST(ScopeDistance, Smoke) {
+  ScopeDistance D({"x::y::z", "x::", "", "a::"});
+  EXPECT_EQ(D.distance("x::y::z::"), 0u);
+  EXPECT_GT(D.distance("x::y::"), D.distance("x::y::z::"));
+  EXPECT_GT(D.distance("x::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance("x::y::z::down::"), D.distance("x::y::"));
+  EXPECT_GT(D.distance(""), D.distance("a::"));
+  EXPECT_GT(D.distance("x::"), D.distance("a::"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
@@ -28,6 +28,7 @@
 #include "llvm/Support/Casting.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -117,13 +118,14 @@
 
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f) << "Decl in current file";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
+  << "Decl in current file";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6f) << "Decl from header";
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 0.6f) << "Decl from header";
   Relevance = {};
   Relevance.merge(CodeCompletionResult(&findDecl(AST, "header_main"), 42));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Current file and header";
 
   auto constructShadowDeclCompletionResult = [&](const std::string DeclName) {
@@ -146,10 +148,10 @@
 
   Relevance = {};
   Relevance.merge(constructShadowDeclCompletionResult("Bar"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
   Relevance.merge(constructShadowDeclCompletionResult("FLAGS_FOO"));
-  EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0f)
+  EXPECT_FLOAT_EQ(Relevance.SemaFileProximityScore, 1.0f)
   << "Using declaration in main file";
 
   Relevance = {};
@@ -210,9 +212,21 @@
   PoorNameMatch.NameMatch = 0.2f;
   EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
 
-  SymbolRelevanceSignals WithSemaProximity;
-  WithSemaProximity.SemaProximityScore = 0.2f;
-  EXPECT_GT(WithSemaProximity.evaluate(), Default.evaluate());
+  SymbolRelevanceSignals WithSemaFileProximity;
+  WithSemaFileProximity.SemaFileProximityScore = 0.2f;
+  EXPECT_GT(WithSemaFileProximity.evaluate(), Default.evaluate());
+
+  ScopeDistance ScopeProximity({"x::y::"});
+
+  SymbolRelevanceSignals WithSemaScopeProximity;
+  WithSemaScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithSemaScopeProximity.SemaSaysInScope = true;
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
+
+  SymbolRelevanceSignals WithIndexScopeProximity;
+  WithIndexScopeProximity.ScopeProximityMatch = &ScopeProximity;
+  WithIndexScopeProximity.SymbolScope = "x::";
+  EXPECT_GT(WithSemaScopeProximity.evaluate(), Default.evaluate());
 
   SymbolRelevanceSignals IndexProximate;
   IndexProximate.SymbolURI = "unittest:/foo/bar.h";
@@ -242,6 +256,35 @@
   EXPECT_EQ(Instance.evaluate(), Default.evaluate());
 }
 
+TEST(QualityTests, ScopeProximity) {
+  SymbolRelevanceSignals Relevance;
+  ScopeDistance ScopeProximity({"x::y::z::", "x::", "llvm::", ""});
+  Relevance.ScopeProximityMatch = &ScopeProximity;
+
+  Relevance.SymbolScope = "other::";
+  float NotMatched = Relevance.evaluate();
+
+  Relevance.SymbolScope = "";
+  float Global = Relevance.evaluate();
+  EXPECT_GT(Global, NotMatched);
+
+  Relevance.SymbolScope = "llvm::

[PATCH] D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address

[Philip Pfaffe via Phabricator via cfe-commits]

philip.pfaffe accepted this revision.
philip.pfaffe added a comment.
This revision is now accepted and ready to land.

LGTM, thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D52814



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(I think your math is off in the description: 20 bits should be 1M lines, not 
4M)
I think this is a win, as I think truncation will be rare and not terrible. We 
should document the intentions around truncation though.

Incidentally, this means replacing just the StringRef in 
SymbolLocation::Position with a char* would save another 13%, because that's 
also 8 bytes.
(Obviously we'd probably replace the other StringRefs too, but it gives a lower 
bound).




Comment at: clangd/index/Index.h:37
+  //
+  // Position is encoded as a 32-bits integer like below:
+  // |<--- 20 bits --->|<-12 bits->|

I think it's enough to say "position is packed into 32 bits to save space".

It'd be useful to spell out the consequences in a second line.



Comment at: clangd/index/Index.h:41
   struct Position {
-uint32_t Line = 0; // 0-based
+static constexpr int LineBits = 20;
+static constexpr int ColumnBits = 12;

(not sure these constants are needed as it stands - YAML shouldn't use them, 
see below)



Comment at: clangd/index/Index.h:46
 // Using UTF-16 code units.
-uint32_t Column = 0; // 0-based
+uint32_t Column : ColumnBits; // 0-based
   };

If we overflow the columns, it would be much easier to recognize if the result 
is always e.g. 255, rather than an essentially random number from 0-255 (from 
modulo 256 arithmetic).

This would mean replacing the raw fields with setters/getters, which is 
obviously a more invasive change. What about a compromise: add the setters, and 
call them from the most important codepaths: `SymbolCollector` and 
`Serialization`.



Comment at: clangd/index/YAMLSerialization.cpp:97
 
-template <> struct MappingTraits {
-  static void mapping(IO &IO, SymbolLocation::Position &Value) {
-IO.mapRequired("Line", Value.Line);
-IO.mapRequired("Column", Value.Column);
+struct NormalizedPosition {
+  using Position = clang::clangd::SymbolLocation::Position;

I don't think there's any reason to pack the YAML encoding.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address

[Fedor Sergeev via Phabricator via cfe-commits]

fedor.sergeev accepted this revision.
fedor.sergeev added a comment.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D52814



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53369: [CodeComplete] Fix accessibility of protected members when accessing members implicitly.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, arphaman.

Repository:
  rC Clang

https://reviews.llvm.org/D53369

Files:
  lib/Sema/SemaCodeComplete.cpp
  test/Index/complete-access-checks.cpp


Index: test/Index/complete-access-checks.cpp
===
--- test/Index/complete-access-checks.cpp
+++ test/Index/complete-access-checks.cpp
@@ -29,8 +29,11 @@
   // RUN: c-index-test -code-completion-at=%s:30:9 %s | FileCheck 
-check-prefix=CHECK-SUPER-ACCESS %s
   this->;
 
+  // RUN: c-index-test -code-completion-at=%s:33:3 %s | FileCheck 
-check-prefix=CHECK-SUPER-ACCESS-IMPLICIT %s
+  
+
   Z that;
-  // RUN: c-index-test -code-completion-at=%s:34:8 %s | FileCheck 
-check-prefix=CHECK-ACCESS %s
+  // RUN: c-index-test -code-completion-at=%s:37:8 %s | FileCheck 
-check-prefix=CHECK-ACCESS %s
   that.
 }
 
@@ -48,6 +51,14 @@
 // CHECK-SUPER-ACCESS: CXXDestructor:{ResultType void}{Informative 
X::}{TypedText ~X}{LeftParen (}{RightParen )} (81)
 // CHECK-SUPER-ACCESS: CXXDestructor:{ResultType void}{TypedText ~Y}{LeftParen 
(}{RightParen )} (79)
 
+// CHECK-SUPER-ACCESS-IMPLICIT: CXXMethod:{ResultType void}{TypedText 
doSomething}{LeftParen (}{RightParen )} (34)
+// CHECK-SUPER-ACCESS-IMPLICIT: CXXMethod:{ResultType void}{TypedText 
func1}{LeftParen (}{RightParen )} (36)
+// CHECK-SUPER-ACCESS-IMPLICIT: CXXMethod:{ResultType void}{TypedText 
func2}{LeftParen (}{RightParen )} (36){{$}}
+// CHECK-SUPER-ACCESS-IMPLICIT: CXXMethod:{ResultType void}{TypedText 
func3}{LeftParen (}{RightParen )} (36) (inaccessible)
+// CHECK-SUPER-ACCESS-IMPLICIT: FieldDecl:{ResultType int}{TypedText member1} 
(37)
+// CHECK-SUPER-ACCESS-IMPLICIT: FieldDecl:{ResultType int}{TypedText member2} 
(37){{$}}
+// CHECK-SUPER-ACCESS-IMPLICIT: FieldDecl:{ResultType int}{TypedText member3} 
(37) (inaccessible)
+
 // CHECK-ACCESS: CXXMethod:{ResultType void}{TypedText func1}{LeftParen 
(}{RightParen )} (34)
 // CHECK-ACCESS: CXXMethod:{ResultType void}{TypedText func2}{LeftParen 
(}{RightParen )} (34) (inaccessible)
 // CHECK-ACCESS: CXXMethod:{ResultType void}{TypedText func3}{LeftParen 
(}{RightParen )} (34) (inaccessible)
@@ -69,9 +80,9 @@
 };
 
 void f(P x, Q y) {
-  // RUN: c-index-test -code-completion-at=%s:73:5 %s | FileCheck 
-check-prefix=CHECK-USING-INACCESSIBLE %s
+  // RUN: c-index-test -code-completion-at=%s:84:5 %s | FileCheck 
-check-prefix=CHECK-USING-INACCESSIBLE %s
   x.; // member is inaccessible
-  // RUN: c-index-test -code-completion-at=%s:75:5 %s | FileCheck 
-check-prefix=CHECK-USING-ACCESSIBLE %s
+  // RUN: c-index-test -code-completion-at=%s:86:5 %s | FileCheck 
-check-prefix=CHECK-USING-ACCESSIBLE %s
   y.; // member is accessible
 }
 
@@ -102,11 +113,11 @@
 };
 
 void D::f(::B *that) {
-  // RUN: c-index-test -code-completion-at=%s:106:9 %s | FileCheck 
-check-prefix=CHECK-PRIVATE-SUPER-THIS %s
+  // RUN: c-index-test -code-completion-at=%s:117:9 %s | FileCheck 
-check-prefix=CHECK-PRIVATE-SUPER-THIS %s
   this->;
 // CHECK-PRIVATE-SUPER-THIS: FieldDecl:{ResultType int}{Informative 
B::}{TypedText member} (37) (inaccessible)
 
-  // RUN: c-index-test -code-completion-at=%s:110:9 %s | FileCheck 
-check-prefix=CHECK-PRIVATE-SUPER-THAT %s
+  // RUN: c-index-test -code-completion-at=%s:121:9 %s | FileCheck 
-check-prefix=CHECK-PRIVATE-SUPER-THAT %s
   that->;
 // CHECK-PRIVATE-SUPER-THAT: FieldDecl:{ResultType int}{TypedText member} (35) 
(inaccessible)
 }
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -3686,13 +3686,20 @@
   }
 
   // If we are in a C++ non-static member function, check the qualifiers on
-  // the member function to filter/prioritize the results list.
-  if (CXXMethodDecl *CurMethod = dyn_cast(CurContext))
-if (CurMethod->isInstance())
+  // the member function to filter/prioritize the results list and set the
+  // context to the record context so that accessibility check in base class
+  // works correctly.
+  RecordDecl *MemberCompletionRecord = nullptr;
+  if (CXXMethodDecl *CurMethod = dyn_cast(CurContext)) {
+if (CurMethod->isInstance()) {
   Results.setObjectTypeQualifiers(
   Qualifiers::fromCVRMask(CurMethod->getTypeQualifiers()));
+  MemberCompletionRecord = CurMethod->getParent();
+}
+  }
 
-  CodeCompletionDeclConsumer Consumer(Results, CurContext);
+  CodeCompletionDeclConsumer Consumer(Results, CurContext, /*FixIts=*/{},
+  MemberCompletionRecord);
   LookupVisibleDecls(S, LookupOrdinaryName, Consumer,
  CodeCompleter->includeGlobals(),
  CodeCompleter->loadExternal());


Index: test/Index/complete-access-checks.cpp
===
--- test/Index/complete-access-checks.cpp
+++ test/I

[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:34
+llvm::APSInt ApInt(/*BitWidth=*/64, /*isUnsigned=*/false);
+ApInt = static_cast(value);
+if (is_negative)

Wouldn't it make more sense to use `std::uint64_t` instead to correspond to the 
line above? And where does the signedness difference come from? 
(`/*isUnsigned=*/false`)



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+  // Don't try and replace things inside of macro definitions.
+  if (!clang::Lexer::makeFileCharRange(
+   clang::CharSourceRange::getCharRange(MatchedCall->getSourceRange()),

That is worth a helper function taking a `SourceRange` as argument.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:69
+  // Macros are ignored.
+  if (Arg->getBeginLoc().isMacroID())
+return;

maybe `assert` instead, as your comment above suggests that macros are already 
filtered out?



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:72
+
+  // Check for static casts to float
+  if (const auto *MaybeCastArg = Result.Nodes.getNodeAs("cast_arg")) {

missing full stop.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:84
+
+  // Check for floats without fractional components
+  if (const auto *LitFloat =

missing full stop



Comment at: docs/clang-tidy/checks/list.rst:12
abseil-redundant-strcat-calls
-   abseil-string-find-startswith
abseil-str-cat-append

spurious change



Comment at: test/clang-tidy/abseil-duration-factory-float.cpp:32
+void ConvertFloatTest() {
+  absl::Duration d;
+

Could you also provide test cases with hexadecimal floating literals, which are 
C++17? The thousand-separators could be checked as well (dunno the correct term 
right now, but the `1'000'000` feature).
Please add test cases for negative literals, too.


https://reviews.llvm.org/D53339



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r339163 - [WebAssembly] Remove use of lld -flavor flag

[Sam Clegg via cfe-commits]

The symlink is correct, the linker's name is just `wasm-ld`.   It is
also the default so you should be able to just drop the -fuse-ld
argument to make your command line work.  I looks like the real fix is
to update the handling of -fuse-ld.
On Tue, Oct 16, 2018 at 9:26 AM Nico Weber  wrote:
>
> $ bin/clang -target wasm32-unknown-unknown -fuse-ld=wasm-ld -o test.wasm 
> test.cc
> clang: error: invalid linker name in argument '-fuse-ld=wasm-ld'
>
> This here http://llvm-cs.pcc.me.uk/tools/clang/lib/Driver/ToolChain.cpp#453 
> makes clang look for "ld.wasm-ld", but the wasm lld symlink is called 
> "wasm-ld". Does the driver need updating? Is the symlink name wrong? Am I 
> doing something wrong?
>
> On Tue, Aug 7, 2018 at 2:56 PM Sam Clegg via cfe-commits 
>  wrote:
>>
>> Author: sbc
>> Date: Tue Aug  7 11:55:41 2018
>> New Revision: 339163
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=339163&view=rev
>> Log:
>> [WebAssembly] Remove use of lld -flavor flag
>>
>> This flag is deprecated. The preferred way to select the lld
>> flavor is by calling it by one of its aliases.
>>
>> Differential Revision: https://reviews.llvm.org/D50395
>>
>> Modified:
>> cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
>> cfe/trunk/lib/Driver/ToolChains/WebAssembly.h
>> cfe/trunk/test/Driver/wasm-toolchain.c
>> cfe/trunk/test/Driver/wasm-toolchain.cpp
>>
>> Modified: cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp?rev=339163&r1=339162&r2=339163&view=diff
>> ==
>> --- cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp (original)
>> +++ cfe/trunk/lib/Driver/ToolChains/WebAssembly.cpp Tue Aug  7 11:55:41 2018
>> @@ -41,8 +41,6 @@ void wasm::Linker::ConstructJob(Compilat
>>const ToolChain &ToolChain = getToolChain();
>>const char *Linker = Args.MakeArgString(ToolChain.GetLinkerPath());
>>ArgStringList CmdArgs;
>> -  CmdArgs.push_back("-flavor");
>> -  CmdArgs.push_back("wasm");
>>
>>if (Args.hasArg(options::OPT_s))
>>  CmdArgs.push_back("--strip-all");
>>
>> Modified: cfe/trunk/lib/Driver/ToolChains/WebAssembly.h
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/WebAssembly.h?rev=339163&r1=339162&r2=339163&view=diff
>> ==
>> --- cfe/trunk/lib/Driver/ToolChains/WebAssembly.h (original)
>> +++ cfe/trunk/lib/Driver/ToolChains/WebAssembly.h Tue Aug  7 11:55:41 2018
>> @@ -66,9 +66,7 @@ private:
>> llvm::opt::ArgStringList &CmdArgs) const 
>> override;
>>std::string getThreadModel() const override;
>>
>> -  const char *getDefaultLinker() const override {
>> -return "lld";
>> -  }
>> +  const char *getDefaultLinker() const override { return "wasm-ld"; }
>>
>>Tool *buildLinker() const override;
>>  };
>>
>> Modified: cfe/trunk/test/Driver/wasm-toolchain.c
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/wasm-toolchain.c?rev=339163&r1=339162&r2=339163&view=diff
>> ==
>> --- cfe/trunk/test/Driver/wasm-toolchain.c (original)
>> +++ cfe/trunk/test/Driver/wasm-toolchain.c Tue Aug  7 11:55:41 2018
>> @@ -12,12 +12,12 @@
>>
>>  // A basic C link command-line.
>>
>> -// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown 
>> --sysroot=/foo -fuse-ld=lld %s 2>&1 | FileCheck -check-prefix=LINK %s
>> +// RUN: %clang -### -no-canonical-prefixes -target wasm32-unknown-unknown 
>> --sysroot=/foo -fuse-ld=wasm-ld %s 2>&1 | FileCheck -check-prefix=LINK %s
>>  // LINK: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
>> -// LINK: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" 
>> "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
>> +// LINK: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" 
>> "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
>>
>>  // A basic C link command-line with optimization.
>>
>> -// RUN: %clang -### -O2 -no-canonical-prefixes -target 
>> wasm32-unknown-unknown --sysroot=/foo -fuse-ld=lld %s 2>&1 | FileCheck 
>> -check-prefix=LINK_OPT %s
>> +// RUN: %clang -### -O2 -no-canonical-prefixes -target 
>> wasm32-unknown-unknown --sysroot=/foo -fuse-ld=wasm-ld %s 2>&1 | FileCheck 
>> -check-prefix=LINK_OPT %s
>>  // LINK_OPT: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
>> -// LINK_OPT: lld{{.*}}" "-flavor" "wasm" "-L/foo/lib" "crt1.o" "[[temp]]" 
>> "-lc" "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
>> +// LINK_OPT: wasm-ld{{.*}}" "-L/foo/lib" "crt1.o" "[[temp]]" "-lc" 
>> "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
>>
>> Modified: cfe/trunk/test/Driver/wasm-toolchain.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/wasm-toolchain.cpp?rev=339163&r1=339162&r2=339163&view=diff
>> 

[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you for the restructurings, i think the code is now way clearer and the 
check close to being done (at least from my side :)).
Could you please mark all notes you consider done as done? Right now i am a bit 
lost on what to track on, as the locations of the notes are not where they were 
as well.

As second thing: Could you please run this check of real world projects? E.g. 
LLVM but maybe others projects you might work on as well and see if you find 
false-postives, bugs or the like.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52988: [analyzer][PlistMacroExpansion] Part 5.: Support for # and ##

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I verified this project on tmux, which uses the preprocessor very heavily. It 
works perfectly, and doesn't crash anywhere despite the **very** liberal use of 
asserts.

In https://reviews.llvm.org/D52988#1267382, @whisperity wrote:

> Looks good.
>
> What happens if the macro is to stringify a partially string argument?
>
>   #define BOOYAH(x) #x ";
>  
>   ... 
>  
>   std::string foo = BOOYAH(blabla)
>   std::string foo2 = BOOYAH("blabla)
>   int x = 2;
>
>
> Not sure if these cases are even valid C(XX), but if they are, we should test.


Lucky, this spawn of a nightmare doesn't compile.

> An idea for a follow-up patch if it's not that hard work: you mentioned your 
> original approach with that madness in the HTML printer. Perhaps it could be 
> refactored to use this implementation too and thus we'll only have 9 places 
> where macro expansion logic is to be maintained, not 10. 😈

The HTML output contains the macro expansions for //all// macros in the file, 
so it's justifiable that entire file is lexed and preprocessed. Granted, using 
`const_cast` and the like (there are some next level hacks in that 
implementation) is risky, but as long as it doesn't break, it does it's job 
better then this solution would.

//As long as it doesn't break.// If you generate a HTML output, the report 
generation speed may not be the greatest concern, so I'll definitely think 
about this a little bit.


https://reviews.llvm.org/D52988



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51866: [analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.
Herald added subscribers: dkrupp, donat.nagy.

Ping


https://reviews.llvm.org/D51866



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Marek Kurdej via Phabricator via cfe-commits]

curdeius created this revision.
curdeius added reviewers: alexfh, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun.

It fixes the false positive when using constexpr if and where else cannot be 
removed:

Example:

  if constexpr (sizeof(int) > 4)
// ...
return /* ... */;
  else // This else cannot be removed.
// ...
return /* ... */;


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372

Files:
  clang-tidy/readability/ElseAfterReturnCheck.cpp
  test/clang-tidy/readability-else-after-return-if-constexpr.cpp


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))


Index: test/clang-tidy/readability-else-after-return-if-constexpr.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-else-after-return-if-constexpr.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -std=c++17
+
+// Constexpr if is an exception to the rule, we cannot remove the else.
+void f() {
+  if (sizeof(int) > 4)
+return;
+  else
+return;
+  // CHECK-MESSAGES: [[@LINE-2]]:3: warning:
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else
+return;
+
+  if constexpr (sizeof(int) > 4)
+return;
+  else if constexpr (sizeof(long) > 4)
+return;
+  else
+return;
+}
+// CHECK-NOT: warning:
Index: clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -25,7 +25,8 @@
  expr(ignoringImplicit(cxxThrowExpr().bind("throw");
   Finder->addMatcher(
   compoundStmt(forEach(
-  ifStmt(hasThen(stmt(
+  ifStmt(unless(isConstexpr()),
+ hasThen(stmt(
  anyOf(ControlFlowInterruptorMatcher,
compoundStmt(has(ControlFlowInterruptorMatcher),
  hasElse(stmt().bind("else")))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 170001.
hokein marked 3 inline comments as done.
hokein added a comment.

Address review comments:

- handle overflowed cases, and added tests
- add getter/setters for line/column and clear all call sides


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363

Files:
  clangd/FindSymbols.cpp
  clangd/XRefs.cpp
  clangd/index/Index.cpp
  clangd/index/Index.h
  clangd/index/Serialization.cpp
  clangd/index/SymbolCollector.cpp
  clangd/index/YAMLSerialization.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/IndexTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -63,19 +63,19 @@
   return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
 }
 MATCHER_P(DeclRange, Pos, "") {
-  return std::tie(arg.CanonicalDeclaration.Start.Line,
-  arg.CanonicalDeclaration.Start.Column,
-  arg.CanonicalDeclaration.End.Line,
-  arg.CanonicalDeclaration.End.Column) ==
- std::tie(Pos.start.line, Pos.start.character, Pos.end.line,
-  Pos.end.character);
+  return std::make_tuple(arg.CanonicalDeclaration.Start.line(),
+ arg.CanonicalDeclaration.Start.column(),
+ arg.CanonicalDeclaration.End.line(),
+ arg.CanonicalDeclaration.End.column()) ==
+ std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
+ Pos.end.character);
 }
 MATCHER_P(DefRange, Pos, "") {
-  return std::tie(arg.Definition.Start.Line,
-  arg.Definition.Start.Column, arg.Definition.End.Line,
-  arg.Definition.End.Column) ==
- std::tie(Pos.start.line, Pos.start.character, Pos.end.line,
-  Pos.end.character);
+  return std::make_tuple(
+ arg.Definition.Start.line(), arg.Definition.Start.column(),
+ arg.Definition.End.line(), arg.Definition.End.column()) ==
+ std::make_tuple(Pos.start.line, Pos.start.character, Pos.end.line,
+ Pos.end.character);
 }
 MATCHER_P(RefCount, R, "") { return int(arg.References) == R; }
 MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
@@ -86,10 +86,10 @@
 MATCHER(RefRange, "") {
   const Ref &Pos = testing::get<0>(arg);
   const Range &Range = testing::get<1>(arg);
-  return std::tie(Pos.Location.Start.Line, Pos.Location.Start.Column,
-  Pos.Location.End.Line, Pos.Location.End.Column) ==
- std::tie(Range.start.line, Range.start.character, Range.end.line,
-  Range.end.character);
+  return std::make_tuple(Pos.Location.Start.line(), Pos.Location.Start.column(),
+ Pos.Location.End.line(), Pos.Location.End.column()) ==
+ std::make_tuple(Range.start.line, Range.start.character,
+ Range.end.line, Range.end.character);
 }
 testing::Matcher &>
 HaveRanges(const std::vector Ranges) {
Index: unittests/clangd/IndexTests.cpp
===
--- unittests/clangd/IndexTests.cpp
+++ unittests/clangd/IndexTests.cpp
@@ -16,6 +16,7 @@
 #include "index/Merge.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 
 using testing::_;
 using testing::AllOf;
@@ -31,13 +32,28 @@
 
 MATCHER_P(Named, N, "") { return arg.Name == N; }
 MATCHER_P(RefRange, Range, "") {
-  return std::tie(arg.Location.Start.Line, arg.Location.Start.Column,
-  arg.Location.End.Line, arg.Location.End.Column) ==
- std::tie(Range.start.line, Range.start.character, Range.end.line,
-  Range.end.character);
+  return std::make_tuple(arg.Location.Start.line(), arg.Location.Start.column(),
+ arg.Location.End.line(), arg.Location.End.column()) ==
+ std::make_tuple(Range.start.line, Range.start.character,
+ Range.end.line, Range.end.character);
 }
 MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
 
+TEST(SymbolLocation, Position) {
+  using Position = SymbolLocation::Position;
+  Position Pos;
+
+  Pos.setLine(1);
+  EXPECT_EQ(1u, Pos.line());
+  Pos.setColumn(2);
+  EXPECT_EQ(2u, Pos.column());
+
+  Pos.setLine((1 << Position::LineBits) + 1); // overflow
+  EXPECT_EQ(Pos.line(), (1u << Position::LineBits) - 1);
+  Pos.setColumn((1 << Position::ColumnBits) + 1); // overflow
+  EXPECT_EQ(Pos.column(), (1u << Position::ColumnBits) - 1);
+}
+
 TEST(SymbolSlab, FindAndIterate) {
   SymbolSlab::Builder B;
   B.insert(symbol("Z"));
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -32,10 +32,10

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D53363#1267628, @sammccall wrote:

> (I think your math is off in the description: 20 bits should be 1M lines, not 
> 4M)


Oops...Update the desccription.

> I think this is a win, as I think truncation will be rare and not terrible. 
> We should document the intentions around truncation though.
> 
> Incidentally, this means replacing just the StringRef in 
> SymbolLocation::Position with a char* would save another 13%, because that's 
> also 8 bytes.

Yeah, I have a rough patch for it, using char* will save us ~50MB memory, which 
will lead to ~300 MB memory usage in total.

> (Obviously we'd probably replace the other StringRefs too, but it gives a 
> lower bound).






Comment at: clangd/index/Index.h:41
   struct Position {
-uint32_t Line = 0; // 0-based
+static constexpr int LineBits = 20;
+static constexpr int ColumnBits = 12;

sammccall wrote:
> (not sure these constants are needed as it stands - YAML shouldn't use them, 
> see below)
I think we need, for testing, for setters, rather than using magic number.



Comment at: clangd/index/Index.h:46
 // Using UTF-16 code units.
-uint32_t Column = 0; // 0-based
+uint32_t Column : ColumnBits; // 0-based
   };

sammccall wrote:
> If we overflow the columns, it would be much easier to recognize if the 
> result is always e.g. 255, rather than an essentially random number from 
> 0-255 (from modulo 256 arithmetic).
> 
> This would mean replacing the raw fields with setters/getters, which is 
> obviously a more invasive change. What about a compromise: add the setters, 
> and call them from the most important codepaths: `SymbolCollector` and 
> `Serialization`.
It seems reasonable to use maximum value if we encounter overflows.

> This would mean replacing the raw fields with setters/getters, which is 
> obviously a more invasive change. What about a compromise: add the setters, 
> and call them from the most important codepaths: SymbolCollector and 
> Serialization.

Actually, there is not too much usage of the raw fields in the source code, I'd 
prefer to use all setters/getters in all places (and it would make the code 
consistent and more safe). How about this plan?

1. I update the patch to change all raw fields usage to getters/setters, and 
keep these raw fields public
2. do a cleanup internally
3. make these raw fields private



Comment at: clangd/index/YAMLSerialization.cpp:97
 
-template <> struct MappingTraits {
-  static void mapping(IO &IO, SymbolLocation::Position &Value) {
-IO.mapRequired("Line", Value.Line);
-IO.mapRequired("Column", Value.Column);
+struct NormalizedPosition {
+  using Position = clang::clangd::SymbolLocation::Position;

sammccall wrote:
> I don't think there's any reason to pack the YAML encoding.

The YAML here is a bit tricky, the previous code could not compile because we 
can not bind bit-field to Non-const references, I added another traits to keep 
the original YAML encoding (which is more readable).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53372: [clang-tidy] Resolve readability-else-after-return false positive for constexpr if.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I think it would be good to add some more explanation as to *why* that `else` 
has to be kept.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53372



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52400: Improve -Wshadow warnings with enumerators

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52400#1267499, @sberg wrote:

> In https://reviews.llvm.org/D52400#1266341, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D52400#1266307, @sberg wrote:
> >
> > >
> >
>
>
> [...]
>
> > Then again, this is a case where you don't get any error but you do get a 
> > silent behavioral ambiguity without the current enumerator shadow 
> > diagnostic:
> > 
> >   struct S1;
> >   struct S2;
> >   struct S3 {
> > void S1();
> > enum { S2 };
> >   
> > void f(decltype(S2) s);
> >   };
> > 
> > 
> > So there are cases where this behavior can be somewhat useful.
>
> but decltype(S2) is a syntax error when S2 names a struct type, no?


Ugh, you're absolutely right.

>>> (ran into such a new -Wshadow while compiling LibreOffice)
>> 
>> Was it a frequent/annoying occurrence?
> 
> there was less than 20 cases overall. about half were "good" warnings about 
> clashing enumerators from different non-scoped enums. the others were 
> "unhelpful" ones about clashes with class names, two of them in stable 
> interface code that cant be changed (so would need ugly #pragma clang warning 
> decorations), one of them even about entities in unrelated include files, so 
> whether a warning is emitted depends on the order in which the files happen 
> to get included in a TU

Thanks, that's good information!

> (and in any case, "declaration shadows a variable" sounds wrong when the 
> shadowed entity is a class type. thats why I thought something is not quite 
> right with this new code yet)

Definitely agreed. I think I'm convinced that this should be silenced when the 
conflict is with a type rather than a value. I'll try to look into this next 
week when I'm back from WG14 meetings.


https://reviews.llvm.org/D52400



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53374: [clangd] Names that are not spelled in source code are reserved.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.

These are often not expected to be used directly e.g.

  TEST_F(Fixture, X) {
^  // "Fixture_X_Test" expanded in the macro should be down ranked.
  }

Only doing this for sema for now, as such symbols are mostly coming from sema
e.g. gtest macros expanded in the main file. We could also add a similar field
for the index symbol.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53374

Files:
  clangd/AST.cpp
  clangd/AST.h
  clangd/Quality.cpp
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -45,7 +45,15 @@
 
 [[deprecated]]
 int _f() { return _X; }
+
+#define DECL_NAME(x, y) x##_##y##_Decl
+#define DECL(x, y) class DECL_NAME(x, y) {};
+DECL(X, Y); // X_Y_Decl
+
+class MAC {};
   )cpp");
+  Header.ExtraArgs = {"-DMAC=mac_name"};
+
   auto Symbols = Header.headerSymbols();
   auto AST = Header.build();
 
@@ -56,6 +64,16 @@
   EXPECT_EQ(Quality.References, SymbolQualitySignals().References);
   EXPECT_EQ(Quality.Category, SymbolQualitySignals::Variable);
 
+  Quality.ReservedName = false;
+  Quality.merge(
+  CodeCompletionResult(&findDecl(AST, "X_Y_Decl"), /*Priority=*/42));
+  EXPECT_TRUE(Quality.ReservedName);
+
+  Quality.ReservedName = false;
+  Quality.merge(
+  CodeCompletionResult(&findDecl(AST, "mac_name"), /*Priority=*/42));
+  EXPECT_TRUE(Quality.ReservedName);
+
   Symbol F = findSymbol(Symbols, "_f");
   F.References = 24; // TestTU doesn't count references, so fake it.
   Quality = {};
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -7,6 +7,7 @@
 //
 //===--===//
 #include "Quality.h"
+#include "AST.h"
 #include "FileDistance.h"
 #include "URI.h"
 #include "index/Index.h"
@@ -178,7 +179,8 @@
 
   if (SemaCCResult.Declaration) {
 if (auto *ID = SemaCCResult.Declaration->getIdentifier())
-  ReservedName = ReservedName || isReserved(ID->getName());
+  ReservedName = ReservedName || isReserved(ID->getName()) ||
+ !SpelledInSourceCode(SemaCCResult.Declaration);
   } else if (SemaCCResult.Kind == CodeCompletionResult::RK_Macro)
 ReservedName = ReservedName || isReserved(SemaCCResult.Macro->getName());
 }
Index: clangd/AST.h
===
--- clangd/AST.h
+++ clangd/AST.h
@@ -24,6 +24,14 @@
 
 namespace clangd {
 
+// Returns true if the complete name of decl \p D is spelled in the source code.
+// This is not the case for
+//   * symbols formed via macro concatenation, the spelling location will
+// be ""
+//   * symbols controlled and defined by a compile command-line option
+// `-DName=foo`, the spelling location will be "".
+bool SpelledInSourceCode(const Decl *D);
+
 /// Find the identifier source location of the given D.
 ///
 /// The returned location is usually the spelling location where the name of the
Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -11,32 +11,34 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
+#include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/USRGeneration.h"
 
 namespace clang {
 namespace clangd {
 using namespace llvm;
 
-SourceLocation findNameLoc(const clang::Decl* D) {
-  const auto& SM = D->getASTContext().getSourceManager();
+bool SpelledInSourceCode(const Decl *D) {
+  const auto &SM = D->getASTContext().getSourceManager();
+  auto Loc = D->getLocation();
   // FIXME: Revisit the strategy, the heuristic is limitted when handling
   // macros, we should use the location where the whole definition occurs.
-  SourceLocation SpellingLoc = SM.getSpellingLoc(D->getLocation());
-  if (D->getLocation().isMacroID()) {
-std::string PrintLoc = SpellingLoc.printToString(SM);
+  if (Loc.isMacroID()) {
+std::string PrintLoc = SM.getSpellingLoc(Loc).printToString(SM);
 if (llvm::StringRef(PrintLoc).startswith("")) {
-  // We use the expansion location for the following symbols, as spelling
-  // locations of these symbols are not interesting to us:
-  //   * symbols formed via macro concatenation, the spelling location will
-  // be ""
-  //   * symbols controlled and defined by a compile command-line option
-  // `-DName=foo`, the spelling location will be "".
-  SpellingLoc = SM.getExpansionRange(D->getLocation()).getBegin();
-}
+llvm::StringRef(PrintLoc).startswith(""))
+  return false;
   }
-  return SpellingLoc;
+  return 

[PATCH] D53377: [clang-tidy] Ignore a case where the fix of make_unique check introduces side effect.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: alexfh.
Herald added a subscriber: xazax.hun.

Previously, ptr.reset(new char[5]) will be replaced with `p =
make_unique(5)`, the fix has side effect -- doing
default initialization, it may cause performace regression (we are
bitten by this rececntly)

The check should be conservative for these cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53377

Files:
  clang-tidy/modernize/MakeSmartPtrCheck.cpp
  test/clang-tidy/modernize-make-unique.cpp


Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -403,18 +403,15 @@
   // CHECK-FIXES: FFs = std::make_unique(Num2);
 
   std::unique_ptr FI;
-  FI.reset(new int[5]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(5);
-  FI.reset(new int[5]());
+  FI.reset(new int[5]()); // default initialization.
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
   // CHECK-FIXES: FI = std::make_unique(5);
+
+  // The check doesn't give warnings and fixes for cases where the original new
+  // expresion doesn't do any initialization.
+  FI.reset(new int[5]);
   FI.reset(new int[Num]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(Num);
   FI.reset(new int[Num2]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(Num2);
 }
 
 void aliases() {
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -121,6 +121,15 @@
   if (New->getNumPlacementArgs() != 0)
 return;
 
+  // Be conservative for cases where we construct an array without any
+  // initalization.
+  // For example,
+  //P.reset(new int[5]) // check fix: P = make_unique(5)
+  //
+  // The fix of the check has side effect, it introduces default initialization
+  // which maybe unexpected and cause performance regression.
+  if (New->isArray() && !New->hasInitializer())
+return;
   if (Construct)
 checkConstruct(SM, Result.Context, Construct, Type, New);
   else if (Reset)


Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -403,18 +403,15 @@
   // CHECK-FIXES: FFs = std::make_unique(Num2);
 
   std::unique_ptr FI;
-  FI.reset(new int[5]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(5);
-  FI.reset(new int[5]());
+  FI.reset(new int[5]()); // default initialization.
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
   // CHECK-FIXES: FI = std::make_unique(5);
+
+  // The check doesn't give warnings and fixes for cases where the original new
+  // expresion doesn't do any initialization.
+  FI.reset(new int[5]);
   FI.reset(new int[Num]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(Num);
   FI.reset(new int[Num2]);
-  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
-  // CHECK-FIXES: FI = std::make_unique(Num2);
 }
 
 void aliases() {
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -121,6 +121,15 @@
   if (New->getNumPlacementArgs() != 0)
 return;
 
+  // Be conservative for cases where we construct an array without any
+  // initalization.
+  // For example,
+  //P.reset(new int[5]) // check fix: P = make_unique(5)
+  //
+  // The fix of the check has side effect, it introduces default initialization
+  // which maybe unexpected and cause performance regression.
+  if (New->isArray() && !New->hasInitializer())
+return;
   if (Construct)
 checkConstruct(SM, Result.Context, Construct, Type, New);
   else if (Reset)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52301: [clang] Set TypeSourceInfo for vardecl's in addition to type when we can deduce.

[Shahms King via Phabricator via cfe-commits]

shahms added a comment.

It's worth noting that `RecursiveASTVisitor` appears to think they are at least 
somewhat equivalent in that it will only visit one or the other and prefers 
`TypeSourceInfo`, if present, via `VisitTypeLoc`.  The fact that the TSI lacks 
this information makes it pretty awkward to access both the recursively deduced 
type and the corresponding source location.  It seems like the places that 
expect this information to be absent could be trivially updated to disregard it 
after this patch, but synthesizing this from the available AST is much more 
challenging.


Repository:
  rC Clang

https://reviews.llvm.org/D52301



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52814: [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address

[Leonard Chan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL344699: [PassManager/Sanitizer] Enable usage of ported 
AddressSanitizer passes with… (authored by leonardchan, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52814?vs=169516&id=170016#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52814

Files:
  cfe/trunk/lib/CodeGen/BackendUtil.cpp
  cfe/trunk/test/CodeGen/asan-new-pm.ll


Index: cfe/trunk/test/CodeGen/asan-new-pm.ll
===
--- cfe/trunk/test/CodeGen/asan-new-pm.ll
+++ cfe/trunk/test/CodeGen/asan-new-pm.ll
@@ -0,0 +1,31 @@
+; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address %s | FileCheck %s
+
+; CHECK: @llvm.global_ctors = {{.*}}@asan.module_ctor
+
+define i32 @test_load(i32* %a) sanitize_address {
+entry:
+; CHECK:  %0 = ptrtoint i32* %a to i64
+; CHECK:  %1 = lshr i64 %0, 3
+; CHECK:  %2 = add i64 %1, 2147450880
+; CHECK:  %3 = inttoptr i64 %2 to i8*
+; CHECK:  %4 = load i8, i8* %3
+; CHECK:  %5 = icmp ne i8 %4, 0
+; CHECK:  br i1 %5, label %6, label %12, !prof !0
+
+; CHECK:; :6:  ; preds = %entry
+; CHECK:  %7 = and i64 %0, 7
+; CHECK:  %8 = add i64 %7, 3
+; CHECK:  %9 = trunc i64 %8 to i8
+; CHECK:  %10 = icmp sge i8 %9, %4
+; CHECK:  br i1 %10, label %11, label %12
+
+; CHECK:; :11: ; preds = %6
+; CHECK:  call void @__asan_report_load4(i64 %0)
+; CHECK:  call void asm sideeffect "", ""()
+; CHECK:  unreachable
+
+; CHECK:; :12: ; preds = %6, %entry
+
+  %tmp1 = load i32, i32* %a, align 4
+  ret i32 %tmp1
+}
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -52,6 +52,7 @@
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Instrumentation.h"
+#include "llvm/Transforms/Instrumentation/AddressSanitizerPass.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
 #include "llvm/Transforms/ObjCARC.h"
@@ -1022,6 +1023,16 @@
CodeGenOpts.DebugPassManager);
   }
 }
+
+if (LangOpts.Sanitize.has(SanitizerKind::Address)) {
+  bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::Address);
+  MPM.addPass(createModuleToFunctionPassAdaptor(
+  AddressSanitizerPass(/*CompileKernel=*/false, Recover,
+   CodeGenOpts.SanitizeAddressUseAfterScope)));
+  bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
+  MPM.addPass(AddressSanitizerPass(/*CompileKernel=*/false, Recover,
+   ModuleUseAfterScope));
+}
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We


Index: cfe/trunk/test/CodeGen/asan-new-pm.ll
===
--- cfe/trunk/test/CodeGen/asan-new-pm.ll
+++ cfe/trunk/test/CodeGen/asan-new-pm.ll
@@ -0,0 +1,31 @@
+; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager -fsanitize=address %s | FileCheck %s
+
+; CHECK: @llvm.global_ctors = {{.*}}@asan.module_ctor
+
+define i32 @test_load(i32* %a) sanitize_address {
+entry:
+; CHECK:  %0 = ptrtoint i32* %a to i64
+; CHECK:  %1 = lshr i64 %0, 3
+; CHECK:  %2 = add i64 %1, 2147450880
+; CHECK:  %3 = inttoptr i64 %2 to i8*
+; CHECK:  %4 = load i8, i8* %3
+; CHECK:  %5 = icmp ne i8 %4, 0
+; CHECK:  br i1 %5, label %6, label %12, !prof !0
+
+; CHECK:; :6:  ; preds = %entry
+; CHECK:  %7 = and i64 %0, 7
+; CHECK:  %8 = add i64 %7, 3
+; CHECK:  %9 = trunc i64 %8 to i8
+; CHECK:  %10 = icmp sge i8 %9, %4
+; CHECK:  br i1 %10, label %11, label %12
+
+; CHECK:; :11: ; preds = %6
+; CHECK:  call void @__asan_report_load4(i64 %0)
+; CHECK:  call void asm sideeffect "", ""()
+; CHECK:  unreachable
+
+; CHECK:; :12: ; preds = %6, %entry
+
+  %tmp1 = load i32, i32* %a, align 4
+  ret i32 %tmp1
+}
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -52,6 +52,7 @@
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Instrumentation.h"
+#include "llvm/Transforms/Instrumentation/AddressSanitizerPass.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
 #include "llvm/Transforms/ObjCARC.h"
@@ -1022,6 +1023

r344699 - [PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with -fsanitize=address

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Wed Oct 17 08:38:22 2018
New Revision: 344699

URL: http://llvm.org/viewvc/llvm-project?rev=344699&view=rev
Log:
[PassManager/Sanitizer] Enable usage of ported AddressSanitizer passes with 
-fsanitize=address

Enable usage of `AddressSanitizer` and `AddressModuleSanitizer` ported from the
legacy to the new PassManager.

This patch depends on https://reviews.llvm.org/D52739.

Differential Revision: https://reviews.llvm.org/D52814

Added:
cfe/trunk/test/CodeGen/asan-new-pm.ll
Modified:
cfe/trunk/lib/CodeGen/BackendUtil.cpp

Modified: cfe/trunk/lib/CodeGen/BackendUtil.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/BackendUtil.cpp?rev=344699&r1=344698&r2=344699&view=diff
==
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp (original)
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp Wed Oct 17 08:38:22 2018
@@ -52,6 +52,7 @@
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Instrumentation.h"
+#include "llvm/Transforms/Instrumentation/AddressSanitizerPass.h"
 #include "llvm/Transforms/Instrumentation/BoundsChecking.h"
 #include "llvm/Transforms/Instrumentation/GCOVProfiler.h"
 #include "llvm/Transforms/ObjCARC.h"
@@ -1022,6 +1023,16 @@ void EmitAssemblyHelper::EmitAssemblyWit
CodeGenOpts.DebugPassManager);
   }
 }
+
+if (LangOpts.Sanitize.has(SanitizerKind::Address)) {
+  bool Recover = CodeGenOpts.SanitizeRecover.has(SanitizerKind::Address);
+  MPM.addPass(createModuleToFunctionPassAdaptor(
+  AddressSanitizerPass(/*CompileKernel=*/false, Recover,
+   CodeGenOpts.SanitizeAddressUseAfterScope)));
+  bool ModuleUseAfterScope = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
+  MPM.addPass(AddressSanitizerPass(/*CompileKernel=*/false, Recover,
+   ModuleUseAfterScope));
+}
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We

Added: cfe/trunk/test/CodeGen/asan-new-pm.ll
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/asan-new-pm.ll?rev=344699&view=auto
==
--- cfe/trunk/test/CodeGen/asan-new-pm.ll (added)
+++ cfe/trunk/test/CodeGen/asan-new-pm.ll Wed Oct 17 08:38:22 2018
@@ -0,0 +1,31 @@
+; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address %s | FileCheck %s
+
+; CHECK: @llvm.global_ctors = {{.*}}@asan.module_ctor
+
+define i32 @test_load(i32* %a) sanitize_address {
+entry:
+; CHECK:  %0 = ptrtoint i32* %a to i64
+; CHECK:  %1 = lshr i64 %0, 3
+; CHECK:  %2 = add i64 %1, 2147450880
+; CHECK:  %3 = inttoptr i64 %2 to i8*
+; CHECK:  %4 = load i8, i8* %3
+; CHECK:  %5 = icmp ne i8 %4, 0
+; CHECK:  br i1 %5, label %6, label %12, !prof !0
+
+; CHECK:; :6:  ; preds = %entry
+; CHECK:  %7 = and i64 %0, 7
+; CHECK:  %8 = add i64 %7, 3
+; CHECK:  %9 = trunc i64 %8 to i8
+; CHECK:  %10 = icmp sge i8 %9, %4
+; CHECK:  br i1 %10, label %11, label %12
+
+; CHECK:; :11: ; preds = %6
+; CHECK:  call void @__asan_report_load4(i64 %0)
+; CHECK:  call void asm sideeffect "", ""()
+; CHECK:  unreachable
+
+; CHECK:; :12: ; preds = %6, %entry
+
+  %tmp1 = load i32, i32* %a, align 4
+  ret i32 %tmp1
+}


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Steffen Kuhn via Phabricator via cfe-commits]

stefson added a comment.

hey there, I've run into problems with stripping static-libs on arm when using 
llvm/clang-7. Could you imagine this patch being at fault?

  strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R 
.GCC.command.line -R .note.gnu.gold-version
 lib/libbz2.so.1.0.6
 usr/bin/bzip2recover
 bin/bzip2
 usr/lib/libbz2.a
  armv7a-unknown-linux-gnueabihf-strip: 
/var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
Failed to find link section for section 11
  armv7a-unknown-linux-gnueabihf-strip: 
/var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
Failed to find link section for section 11


Repository:
  rC Clang

https://reviews.llvm.org/D46013



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

[Steffen Kuhn via Phabricator via cfe-commits]

stefson added a comment.
Herald added a reviewer: javed.absar.
Herald added subscribers: dexonsmith, chrib.

hey there, I've run into problems with stripping static-libs on arm when using 
llvm/clang-7. Could you imagine this patch being at fault?

  strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R 
.GCC.command.line -R .note.gnu.gold-version
 lib/libbz2.so.1.0.6
 usr/bin/bzip2recover
 bin/bzip2
 usr/lib/libbz2.a
  armv7a-unknown-linux-gnueabihf-strip: 
/var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
Failed to find link section for section 11
  armv7a-unknown-linux-gnueabihf-strip: 
/var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
Failed to find link section for section 11


Repository:
  rL LLVM

https://reviews.llvm.org/D45240



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D45240#1267846, @stefson wrote:

> hey there, I've run into problems with stripping static-libs on arm when 
> using llvm/clang-7. Could you imagine this patch being at fault?
>
>   strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R 
> .GCC.command.line -R .note.gnu.gold-version
>   lib/libbz2.so.1.0.6
>   usr/bin/bzip2recover
>   bin/bzip2
>   usr/lib/libbz2.a
>armv7a-unknown-linux-gnueabihf-strip: 
> /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
> Failed to find link section for section 11
>armv7a-unknown-linux-gnueabihf-strip: 
> /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
> Failed to find link section for section 11


It doesn't seem that likely to me. The error you are seeing looks like a broken 
object file, the patch here will affect the type of features a target might use 
during code generation but it shouldn't affect strip.

I suggest dumping the ELF file with readelf or objdump and look at the section 
output for section 11. Some sections have a special meaning for the sh_link 
field (http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link) it 
looks like strip is complaining that either a section is missing a sh_link 
field that it should have or it has an invalid index.

It will also be worth seeing if the objects in that library have been processed 
by any other tool such as objcpy to see if it has introduced an error.


Repository:
  rL LLVM

https://reviews.llvm.org/D45240



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r344701 - Fix for failing unit tests on some bots after r344696.

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Wed Oct 17 09:21:19 2018
New Revision: 344701

URL: http://llvm.org/viewvc/llvm-project?rev=344701&view=rev
Log:
Fix for failing unit tests on some bots after r344696.

Modified:
cfe/trunk/test/CodeGen/asan-new-pm.ll

Modified: cfe/trunk/test/CodeGen/asan-new-pm.ll
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/asan-new-pm.ll?rev=344701&r1=344700&r2=344701&view=diff
==
--- cfe/trunk/test/CodeGen/asan-new-pm.ll (original)
+++ cfe/trunk/test/CodeGen/asan-new-pm.ll Wed Oct 17 09:21:19 2018
@@ -1,31 +1,10 @@
-; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address %s | FileCheck %s
+; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address -triple=x86_64-linux %s | FileCheck %s
 
 ; CHECK: @llvm.global_ctors = {{.*}}@asan.module_ctor
+; CHECK: declare void @__asan_loadN
 
 define i32 @test_load(i32* %a) sanitize_address {
 entry:
-; CHECK:  %0 = ptrtoint i32* %a to i64
-; CHECK:  %1 = lshr i64 %0, 3
-; CHECK:  %2 = add i64 %1, 2147450880
-; CHECK:  %3 = inttoptr i64 %2 to i8*
-; CHECK:  %4 = load i8, i8* %3
-; CHECK:  %5 = icmp ne i8 %4, 0
-; CHECK:  br i1 %5, label %6, label %12, !prof !0
-
-; CHECK:; :6:  ; preds = %entry
-; CHECK:  %7 = and i64 %0, 7
-; CHECK:  %8 = add i64 %7, 3
-; CHECK:  %9 = trunc i64 %8 to i8
-; CHECK:  %10 = icmp sge i8 %9, %4
-; CHECK:  br i1 %10, label %11, label %12
-
-; CHECK:; :11: ; preds = %6
-; CHECK:  call void @__asan_report_load4(i64 %0)
-; CHECK:  call void asm sideeffect "", ""()
-; CHECK:  unreachable
-
-; CHECK:; :12: ; preds = %6, %entry
-
   %tmp1 = load i32, i32* %a, align 4
   ret i32 %tmp1
 }


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53339: [clang-tidy] Add the abseil-duration-factory-float check

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:24
+truncateIfIntegral(const FloatingLiteral &FloatLiteral) {
+  double value = FloatLiteral.getValueAsApproximateDouble();
+  if (std::fmod(value, 1) == 0) {

All variables (local, global, function parameter) use exactly same naming 
convention `CamelCase`.


https://reviews.llvm.org/D53339



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r344702 - Fix for arm bots afternew PM pass port. Prevent cross compiling on arm.

[Leonard Chan via cfe-commits]

Author: leonardchan
Date: Wed Oct 17 11:12:18 2018
New Revision: 344702

URL: http://llvm.org/viewvc/llvm-project?rev=344702&view=rev
Log:
Fix for arm bots afternew PM pass port. Prevent cross compiling on arm.

Modified:
cfe/trunk/test/CodeGen/asan-new-pm.ll

Modified: cfe/trunk/test/CodeGen/asan-new-pm.ll
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/asan-new-pm.ll?rev=344702&r1=344701&r2=344702&view=diff
==
--- cfe/trunk/test/CodeGen/asan-new-pm.ll (original)
+++ cfe/trunk/test/CodeGen/asan-new-pm.ll Wed Oct 17 11:12:18 2018
@@ -1,4 +1,4 @@
-; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address -triple=x86_64-linux %s | FileCheck %s
+; RUN: %clang_cc1 -S -emit-llvm -o - -fexperimental-new-pass-manager 
-fsanitize=address %s | FileCheck %s
 
 ; CHECK: @llvm.global_ctors = {{.*}}@asan.module_ctor
 ; CHECK: declare void @__asan_loadN


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Starting multiple threads on the LLVM mailing lists with the same message is 
spam.  Please don't do that.

If you think you've run into a bug, file a bug report at bugs.llvm.org.  For 
general questions, send an email to llvm-dev.


Repository:
  rC Clang

https://reviews.llvm.org/D46013



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53377: [clang-tidy] Ignore a case where the fix of make_unique check introduces side effect.

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons accepted this revision.
malcolm.parsons added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53377



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53381: [clang-doc] Bringing bitcode tests in line

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added reviewers: leonardchan, jakehehrlich, lebedev.ri.
juliehockett added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman.

Makes bitcode tests line up with what's actually called in the tool. Should fix 
the failing bot.
Also fixes a warning that was being thrown about initialization braces.


https://reviews.llvm.org/D53381

Files:
  clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
  clang-tools-extra/unittests/clang-doc/ClangDocTest.h


Index: clang-tools-extra/unittests/clang-doc/ClangDocTest.h
===
--- clang-tools-extra/unittests/clang-doc/ClangDocTest.h
+++ clang-tools-extra/unittests/clang-doc/ClangDocTest.h
@@ -22,7 +22,7 @@
 
 static const SymbolID EmptySID = SymbolID();
 static const SymbolID NonEmptySID =
-SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
+SymbolID{{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}};
 
 NamespaceInfo *InfoAsNamespace(Info *I);
 RecordInfo *InfoAsRecord(Info *I);
Index: clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
@@ -18,15 +18,29 @@
 namespace clang {
 namespace doc {
 
-std::string writeInfo(Info *I) {
+template  static std::string writeInfo(T &I) {
   SmallString<2048> Buffer;
   llvm::BitstreamWriter Stream(Buffer);
   ClangDocBitcodeWriter Writer(Stream);
-  // Check that there was no error in the write.
-  assert(Writer.dispatchInfoForWrite(I) == false);
+  Writer.emitBlock(I);
   return Buffer.str().str();
 }
 
+std::string writeInfo(Info *I) {
+  switch (I->IT) {
+  case InfoType::IT_namespace:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_record:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_enum:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_function:
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }
+}
+
 std::vector> readInfo(StringRef Bitcode,
 size_t NumInfos) {
   llvm::BitstreamCursor Stream(Bitcode);


Index: clang-tools-extra/unittests/clang-doc/ClangDocTest.h
===
--- clang-tools-extra/unittests/clang-doc/ClangDocTest.h
+++ clang-tools-extra/unittests/clang-doc/ClangDocTest.h
@@ -22,7 +22,7 @@
 
 static const SymbolID EmptySID = SymbolID();
 static const SymbolID NonEmptySID =
-SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
+SymbolID{{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}};
 
 NamespaceInfo *InfoAsNamespace(Info *I);
 RecordInfo *InfoAsRecord(Info *I);
Index: clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp
@@ -18,15 +18,29 @@
 namespace clang {
 namespace doc {
 
-std::string writeInfo(Info *I) {
+template  static std::string writeInfo(T &I) {
   SmallString<2048> Buffer;
   llvm::BitstreamWriter Stream(Buffer);
   ClangDocBitcodeWriter Writer(Stream);
-  // Check that there was no error in the write.
-  assert(Writer.dispatchInfoForWrite(I) == false);
+  Writer.emitBlock(I);
   return Buffer.str().str();
 }
 
+std::string writeInfo(Info *I) {
+  switch (I->IT) {
+  case InfoType::IT_namespace:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_record:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_enum:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_function:
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }
+}
+
 std::vector> readInfo(StringRef Bitcode,
 size_t NumInfos) {
   llvm::BitstreamCursor Stream(Bitcode);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r344650 - [clang-doc] Add unit tests for serialization

[Julie Hockett via cfe-commits]

https://reviews.llvm.org/D53381 should fix this -- thanks for the note!

Julie

On Wed, Oct 17, 2018 at 1:58 AM Mikael Holmén 
wrote:

> Hi Julie,
>
> clang 3.6.0 complains on this commit:
>
> /usr/bin/clang++  -march=corei7  -DGTEST_HAS_RTTI=0
> -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_LANG_CXX11=1 -D_DEBUG -D_GNU_SOURCE
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
> -Itools/clang/tools/extra/unittests/clang-doc
> -I../tools/clang/tools/extra/unittests/clang-doc
> -I../tools/clang/include -Itools/clang/include -I/usr/include/libxml2
> -Iinclude -I../include -I../tools/clang/tools/extra/clang-doc
> -I../utils/unittest/googletest/include
> -I../utils/unittest/googlemock/include
> -I/proj/flexasic/app/valgrind/3.11.0/include  -fPIC
> -fvisibility-inlines-hidden -Werror -Werror=date-time -std=c++11 -Wall
> -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual
> -Wmissing-field-initializers -pedantic -Wno-long-long
> -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor
> -Wstring-conversion -fdiagnostics-color -ffunction-sections
> -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types
> -O3-UNDEBUG  -Wno-variadic-macros
> -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -MMD
> -MT
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
>
> -MF
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o.d
>
> -o
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
>
> -c ../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp
> In file included from
> ../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp:12:
> ../tools/clang/tools/extra/unittests/clang-doc/ClangDocTest.h:25:14:
> error: suggest braces around initialization of subobject
> [-Werror,-Wmissing-braces]
>  SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
>   ^~
>   { }
> 1 error generated.
>
> Regards,
> Mikael
>
> On 10/17/2018 01:06 AM, Julie Hockett via cfe-commits wrote:
> > Author: juliehockett
> > Date: Tue Oct 16 16:06:42 2018
> > New Revision: 344650
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=344650&view=rev
> > Log:
> > [clang-doc] Add unit tests for serialization
> >
> > Adds unit tests for the Serialize library.
> >
> > This is part of a move to convert clang-doc's tests to a more
> > maintainable unit test framework, with a smaller number of integration
> > tests to maintain and more granular failure feedback.
> >
> > Differential Revision: https://reviews.llvm.org/D53081
> >
> > Added:
> >  clang-tools-extra/trunk/unittests/clang-doc/
> >  clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
> >  clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp
> >  clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
> >  clang-tools-extra/trunk/unittests/clang-doc/SerializeTest.cpp
> > Modified:
> >  clang-tools-extra/trunk/unittests/CMakeLists.txt
> >
> > Modified: clang-tools-extra/trunk/unittests/CMakeLists.txt
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/CMakeLists.txt?rev=344650&r1=344649&r2=344650&view=diff
> >
> ==
> > --- clang-tools-extra/trunk/unittests/CMakeLists.txt (original)
> > +++ clang-tools-extra/trunk/unittests/CMakeLists.txt Tue Oct 16 16:06:42
> 2018
> > @@ -16,6 +16,7 @@ endif()
> >
> >   add_subdirectory(change-namespace)
> >   add_subdirectory(clang-apply-replacements)
> > +add_subdirectory(clang-doc)
> >   add_subdirectory(clang-move)
> >   add_subdirectory(clang-query)
> >   add_subdirectory(clang-tidy)
> >
> > Added: clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
> > URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt?rev=344650&view=auto
> >
> ==
> > --- clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt (added)
> > +++ clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt Tue Oct
> 16 16:06:42 2018
> > @@ -0,0 +1,29 @@
> > +set(LLVM_LINK_COMPONENTS
> > +  support
> > +  BitReader
> > +  BitWriter
> > +  )
> > +
> > +get_filename_component(CLANG_DOC_SOURCE_DIR
> > +  ${CMAKE_CURRENT_SOURCE_DIR}/../../clang-doc REALPATH)
> > +include_directories(
> > +  ${CLANG_DOC_SOURCE_DIR}
> > +  )
> > +
> > +add_extra_unittest(ClangDocTests
> > +  ClangDocTest.cpp
> > +  SerializeTest.cpp
> > +  )
> > +
> > +target_link_libraries(ClangDocTests
> > +  PRIVATE
> > +  clangAST
> > +  clangASTMatchers
> > +  clangBasic
> > +  clangDoc
> > +  clangFormat
> > +  clangFrontend
> > +  clangRewrite
> > +  clangTooling
> > +  clangToolingCore
> > +  )
> >
> > Added: clang-too

[PATCH] D53382: [clang-doc] Update documentation

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added reviewers: leonardchan, jakehehrlich, lebedev.ri.
juliehockett added a project: clang-tools-extra.

https://reviews.llvm.org/D53382

Files:
  clang-tools-extra/docs/clang-doc.rst


Index: clang-tools-extra/docs/clang-doc.rst
===
--- clang-tools-extra/docs/clang-doc.rst
+++ clang-tools-extra/docs/clang-doc.rst
@@ -17,27 +17,43 @@
 there.
 
 Use
-=
+
 
 :program:`clang-doc` is a `LibTooling
 `_-based tool, and so requires a
 compile command database for your project (for an example of how to do this 
 see `How To Setup Tooling For LLVM
 `_).
 
-The tool can be used on a single file or multiple files as defined in 
-the compile commands database:
+By default, the tool will run on all files listed in the given compile commands
+database:
 
 .. code-block:: console
 
-  $ clang-doc /path/to/file.cpp -p /path/to/compile/commands
+  $ clang-doc /path/to/compile_commands.json
 
-This generates an intermediate representation of the declarations and their
-associated information in the specified TUs, serialized to LLVM bitcode.
+Output
+===
 
-As currently implemented, the tool is only able to parse TUs that can be 
-stored in-memory. Future additions will extend the current framework to use
-map-reduce frameworks to allow for use with large codebases.
+:program:`clang-doc` produces a directory of documentation. One file is 
produced
+for each namespace and record in the project source code, containing all
+documentation (including contained functions, methods, and enums) for that 
item.
+
+The top-level directory is configurable through the ``output`` flag: 
+
+.. code-block:: console
+
+  $ clang-doc -output=output/directory/ compile_commands.json
+
+Configuration
+==
+
+Configuration for :program:`clang-doc` is currently limited to command-line 
options.
+In the future, it may develop the ability to use a configuration file, but no 
such
+efforts are currently in progress.
+
+Options
+
 
 :program:`clang-doc` offers the following options:
 
@@ -57,9 +73,11 @@
   clang-doc options:
 
 -doxygen   - Use only doxygen-style comments to generate 
docs.
--dump  - Dump intermediate results to bitcode file.
 -extra-arg=- Additional argument to append to the compiler 
command line
 -extra-arg-before= - Additional argument to prepend to the 
compiler command line
--omit-filenames- Omit filenames in output.
+-format- Format for outputted docs.
+  =yaml-   Documentation in YAML format.
+  =md  -   Documentation in MD format.
 -output=   - Directory for outputting generated files.
 -p=- Build path
+-public- Document only public declarations.


Index: clang-tools-extra/docs/clang-doc.rst
===
--- clang-tools-extra/docs/clang-doc.rst
+++ clang-tools-extra/docs/clang-doc.rst
@@ -17,27 +17,43 @@
 there.
 
 Use
-=
+
 
 :program:`clang-doc` is a `LibTooling
 `_-based tool, and so requires a
 compile command database for your project (for an example of how to do this 
 see `How To Setup Tooling For LLVM
 `_).
 
-The tool can be used on a single file or multiple files as defined in 
-the compile commands database:
+By default, the tool will run on all files listed in the given compile commands
+database:
 
 .. code-block:: console
 
-  $ clang-doc /path/to/file.cpp -p /path/to/compile/commands
+  $ clang-doc /path/to/compile_commands.json
 
-This generates an intermediate representation of the declarations and their
-associated information in the specified TUs, serialized to LLVM bitcode.
+Output
+===
 
-As currently implemented, the tool is only able to parse TUs that can be 
-stored in-memory. Future additions will extend the current framework to use
-map-reduce frameworks to allow for use with large codebases.
+:program:`clang-doc` produces a directory of documentation. One file is produced
+for each namespace and record in the project source code, containing all
+documentation (including contained functions, methods, and enums) for that item.
+
+The top-level directory is configurable through the ``output`` flag: 
+
+.. code-block:: console
+
+  $ clang-doc -output=output/directory/ compile_commands.json
+
+Configuration
+==
+
+Configuration for :program:`clang-doc` is currently limited to command-line options.
+In the future, it may develop the ability to use a configuration file, but no such
+efforts are currently in progress.
+
+Options
+
 
 :program:`clang-doc` offers the following opt

[PATCH] D53381: [clang-doc] Bringing bitcode tests in line

[Leonard Chan via Phabricator via cfe-commits]

leonardchan accepted this revision.
leonardchan added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp:39-40
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }

Nit: The `dispatchInfoForWrite` function returns true on default and writeInfo 
would catch that assert. I assume just returning empty string is intended on 
this case?


https://reviews.llvm.org/D53381



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r344707 - [clang-doc] Bringing bitcode tests in line

[Julie Hockett via cfe-commits]

Author: juliehockett
Date: Wed Oct 17 13:16:05 2018
New Revision: 344707

URL: http://llvm.org/viewvc/llvm-project?rev=344707&view=rev
Log:
[clang-doc] Bringing bitcode tests in line

Makes bitcode tests line up with what's actually called in the tool.
Should fix the failing bot.

Also fixes a warning that was being thrown about initialization braces.

Differential Revision: https://reviews.llvm.org/D53381

Modified:
clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h

Modified: clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp?rev=344707&r1=344706&r2=344707&view=diff
==
--- clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp (original)
+++ clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp Wed Oct 17 
13:16:05 2018
@@ -18,15 +18,29 @@
 namespace clang {
 namespace doc {
 
-std::string writeInfo(Info *I) {
+template  static std::string writeInfo(T &I) {
   SmallString<2048> Buffer;
   llvm::BitstreamWriter Stream(Buffer);
   ClangDocBitcodeWriter Writer(Stream);
-  // Check that there was no error in the write.
-  assert(Writer.dispatchInfoForWrite(I) == false);
+  Writer.emitBlock(I);
   return Buffer.str().str();
 }
 
+std::string writeInfo(Info *I) {
+  switch (I->IT) {
+  case InfoType::IT_namespace:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_record:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_enum:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_function:
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }
+}
+
 std::vector> readInfo(StringRef Bitcode,
 size_t NumInfos) {
   llvm::BitstreamCursor Stream(Bitcode);

Modified: clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h?rev=344707&r1=344706&r2=344707&view=diff
==
--- clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h (original)
+++ clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h Wed Oct 17 
13:16:05 2018
@@ -22,7 +22,7 @@ using EmittedInfoList = std::vectorhttp://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53381: [clang-doc] Bringing bitcode tests in line

[Julie Hockett via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
juliehockett marked an inline comment as done.
Closed by commit rL344707: [clang-doc] Bringing bitcode tests in line (authored 
by juliehockett, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53381?vs=170024&id=170033#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53381

Files:
  clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
  clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h


Index: clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
===
--- clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
+++ clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
@@ -22,7 +22,7 @@
 
 static const SymbolID EmptySID = SymbolID();
 static const SymbolID NonEmptySID =
-SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
+SymbolID{{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}};
 
 NamespaceInfo *InfoAsNamespace(Info *I);
 RecordInfo *InfoAsRecord(Info *I);
Index: clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
@@ -18,15 +18,29 @@
 namespace clang {
 namespace doc {
 
-std::string writeInfo(Info *I) {
+template  static std::string writeInfo(T &I) {
   SmallString<2048> Buffer;
   llvm::BitstreamWriter Stream(Buffer);
   ClangDocBitcodeWriter Writer(Stream);
-  // Check that there was no error in the write.
-  assert(Writer.dispatchInfoForWrite(I) == false);
+  Writer.emitBlock(I);
   return Buffer.str().str();
 }
 
+std::string writeInfo(Info *I) {
+  switch (I->IT) {
+  case InfoType::IT_namespace:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_record:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_enum:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_function:
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }
+}
+
 std::vector> readInfo(StringRef Bitcode,
 size_t NumInfos) {
   llvm::BitstreamCursor Stream(Bitcode);


Index: clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
===
--- clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
+++ clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
@@ -22,7 +22,7 @@
 
 static const SymbolID EmptySID = SymbolID();
 static const SymbolID NonEmptySID =
-SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
+SymbolID{{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}};
 
 NamespaceInfo *InfoAsNamespace(Info *I);
 RecordInfo *InfoAsRecord(Info *I);
Index: clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/BitcodeTest.cpp
@@ -18,15 +18,29 @@
 namespace clang {
 namespace doc {
 
-std::string writeInfo(Info *I) {
+template  static std::string writeInfo(T &I) {
   SmallString<2048> Buffer;
   llvm::BitstreamWriter Stream(Buffer);
   ClangDocBitcodeWriter Writer(Stream);
-  // Check that there was no error in the write.
-  assert(Writer.dispatchInfoForWrite(I) == false);
+  Writer.emitBlock(I);
   return Buffer.str().str();
 }
 
+std::string writeInfo(Info *I) {
+  switch (I->IT) {
+  case InfoType::IT_namespace:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_record:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_enum:
+return writeInfo(*static_cast(I));
+  case InfoType::IT_function:
+return writeInfo(*static_cast(I));
+  default:
+return "";
+  }
+}
+
 std::vector> readInfo(StringRef Bitcode,
 size_t NumInfos) {
   llvm::BitstreamCursor Stream(Bitcode);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51429: [AArch64] Return Address Signing B Key Support

[Oliver Stannard via Phabricator via cfe-commits]

olista01 added inline comments.



Comment at: lib/CodeGen/CGDeclCXX.cpp:364
   if (RASignKind != CodeGenOptions::SignReturnAddressScope::None)
+  {
 Fn->addFnAttr("sign-return-address",

LLVM style has the opening brace on the same line as the if. There's a 
git-clang-format script in the clang repo which can fix up style issues like 
this for a whole patch, changing touching lines you haven't changes.



Comment at: lib/CodeGen/CGDeclCXX.cpp:374
+  : "b_key");
+  }
+

I think we need to handle branch-target-enforcement here too.



Comment at: lib/CodeGen/TargetInfo.cpp:4982
+if (Kind != CodeGenOptions::SignReturnAddressScope::None)
+{
+  Fn->addFnAttr("sign-return-address",

Style nit: brace on same line as if.



Comment at: lib/Driver/ToolChains/Clang.cpp:1433
 
+static std::tuple
+ParseAArch64BranchProtection(const Driver &D, const ArgList &Args, const Arg 
*A) {

Please add a comment about what the parts of the return value are.



Comment at: lib/Driver/ToolChains/Clang.cpp:1440
+  StringRef Value = A->getValue();
+  // This maps onto -mbranch-protection=+
+

I'm not sure what this is trying to say. Is it referring to the "standard" case 
below?



Comment at: lib/Driver/ToolChains/Clang.cpp:1448
+  } else if (!Value.equals("none")) {
+SmallVector BranchProtection;
+StringRef(A->getValue()).split(BranchProtection, '+');

I'd make this 4, because that's the longest sensible argument 
("pac-ret+leaf+b-key+bti").



Comment at: lib/Driver/ToolChains/Clang.cpp:1460
+Scope = "non-leaf";
+while (++Protection != BranchProtection.end()) {
+  if (Protection->equals("leaf"))

Add a comment about the fact that "leaf" and "b-key" must follow "pac-ret", to 
explain why we don't just parse this in one loop.



Comment at: lib/Driver/ToolChains/Clang.cpp:1543
+if (A->getOption().matches(options::OPT_msign_return_address_EQ))
+{
+  Scope = A->getValue();

Nit: brace on same line as if.



Comment at: test/CodeGenCXX/aarch64-sign-return-address-static-ctor.cpp:7
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-ALL  
--check-prefix=CHECK-A-KEY
 
 struct Foo {

This should also test branch target enforcement (it's also missing from the 
code).


https://reviews.llvm.org/D51429



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r344710 - [AArch64] Define __ELF__ for aarch64-none-elf and other similar triples.

[Eli Friedman via cfe-commits]

Author: efriedma
Date: Wed Oct 17 14:07:11 2018
New Revision: 344710

URL: http://llvm.org/viewvc/llvm-project?rev=344710&view=rev
Log:
[AArch64] Define __ELF__ for aarch64-none-elf and other similar triples.

"aarch64-none-elf" is commonly used for AArch64 baremetal toolchains.

Differential Revision: https://reviews.llvm.org/D53348


Modified:
cfe/trunk/lib/Basic/Targets/AArch64.cpp
cfe/trunk/test/Preprocessor/init.c

Modified: cfe/trunk/lib/Basic/Targets/AArch64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/AArch64.cpp?rev=344710&r1=344709&r2=344710&view=diff
==
--- cfe/trunk/lib/Basic/Targets/AArch64.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/AArch64.cpp Wed Oct 17 14:07:11 2018
@@ -122,10 +122,9 @@ void AArch64TargetInfo::getTargetDefines
  MacroBuilder &Builder) const {
   // Target identification.
   Builder.defineMacro("__aarch64__");
-  // For bare-metal none-eabi.
+  // For bare-metal.
   if (getTriple().getOS() == llvm::Triple::UnknownOS &&
-  (getTriple().getEnvironment() == llvm::Triple::EABI ||
-   getTriple().getEnvironment() == llvm::Triple::EABIHF))
+  getTriple().isOSBinFormatELF())
 Builder.defineMacro("__ELF__");
 
   // Target properties.

Modified: cfe/trunk/test/Preprocessor/init.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/init.c?rev=344710&r1=344709&r2=344710&view=diff
==
--- cfe/trunk/test/Preprocessor/init.c (original)
+++ cfe/trunk/test/Preprocessor/init.c Wed Oct 17 14:07:11 2018
@@ -2590,6 +2590,7 @@
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=arm-none-eabihf < /dev/null | 
FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabi < /dev/null 
| FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabihf < 
/dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-elf < /dev/null 
| FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // ARM-NONE-EABI: #define __ELF__ 1
 
 // No MachO targets use the full EABI, even if AAPCS is used.


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53348: [AArch64] Define __ELF__ for aarch64-none-elf and other similar triples.

[Eli Friedman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344710: [AArch64] Define __ELF__ for aarch64-none-elf and 
other similar triples. (authored by efriedma, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D53348

Files:
  lib/Basic/Targets/AArch64.cpp
  test/Preprocessor/init.c


Index: lib/Basic/Targets/AArch64.cpp
===
--- lib/Basic/Targets/AArch64.cpp
+++ lib/Basic/Targets/AArch64.cpp
@@ -122,10 +122,9 @@
  MacroBuilder &Builder) const {
   // Target identification.
   Builder.defineMacro("__aarch64__");
-  // For bare-metal none-eabi.
+  // For bare-metal.
   if (getTriple().getOS() == llvm::Triple::UnknownOS &&
-  (getTriple().getEnvironment() == llvm::Triple::EABI ||
-   getTriple().getEnvironment() == llvm::Triple::EABIHF))
+  getTriple().isOSBinFormatELF())
 Builder.defineMacro("__ELF__");
 
   // Target properties.
Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -2590,6 +2590,7 @@
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=arm-none-eabihf < /dev/null | 
FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabi < /dev/null 
| FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabihf < 
/dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-elf < /dev/null 
| FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // ARM-NONE-EABI: #define __ELF__ 1
 
 // No MachO targets use the full EABI, even if AAPCS is used.


Index: lib/Basic/Targets/AArch64.cpp
===
--- lib/Basic/Targets/AArch64.cpp
+++ lib/Basic/Targets/AArch64.cpp
@@ -122,10 +122,9 @@
  MacroBuilder &Builder) const {
   // Target identification.
   Builder.defineMacro("__aarch64__");
-  // For bare-metal none-eabi.
+  // For bare-metal.
   if (getTriple().getOS() == llvm::Triple::UnknownOS &&
-  (getTriple().getEnvironment() == llvm::Triple::EABI ||
-   getTriple().getEnvironment() == llvm::Triple::EABIHF))
+  getTriple().isOSBinFormatELF())
 Builder.defineMacro("__ELF__");
 
   // Target properties.
Index: test/Preprocessor/init.c
===
--- test/Preprocessor/init.c
+++ test/Preprocessor/init.c
@@ -2590,6 +2590,7 @@
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=arm-none-eabihf < /dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabi < /dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-eabihf < /dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=aarch64-none-elf < /dev/null | FileCheck -match-full-lines -check-prefix ARM-NONE-EABI %s
 // ARM-NONE-EABI: #define __ELF__ 1
 
 // No MachO targets use the full EABI, even if AAPCS is used.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r344711 - AMDGPU: Add options to enable/disable code object v3

[Konstantin Zhuravlyov via cfe-commits]

Author: kzhuravl
Date: Wed Oct 17 14:39:12 2018
New Revision: 344711

URL: http://llvm.org/viewvc/llvm-project?rev=344711&view=rev
Log:
AMDGPU: Add options to enable/disable code object v3

Differential Revision: https://reviews.llvm.org/D53386

Modified:
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/test/Driver/amdgpu-features.c

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=344711&r1=344710&r2=344711&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed Oct 17 14:39:12 2018
@@ -2085,6 +2085,11 @@ def mamdgpu_debugger_abi : Joined<["-"],
   Group,
   HelpText<"Generate additional code for specified  of debugger ABI 
(AMDGPU only)">,
   MetaVarName<"">;
+
+def mcode_object_v3 : Flag<["-"], "mcode-object-v3">, 
Group,
+  HelpText<"Enable code object v3 (AMDGPU only)">;
+def mno_code_object_v3 : Flag<["-"], "mno-code-object-v3">, 
Group,
+  HelpText<"Disable code object v3 (AMDGPU only)">;
 def mxnack : Flag<["-"], "mxnack">, Group,
   HelpText<"Enable XNACK (AMDGPU only)">;
 def mno_xnack : Flag<["-"], "mno-xnack">, Group,

Modified: cfe/trunk/test/Driver/amdgpu-features.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/amdgpu-features.c?rev=344711&r1=344710&r2=344711&view=diff
==
--- cfe/trunk/test/Driver/amdgpu-features.c (original)
+++ cfe/trunk/test/Driver/amdgpu-features.c Wed Oct 17 14:39:12 2018
@@ -6,6 +6,12 @@
 // RUN:   | FileCheck --check-prefix=CHECK-MAMDGPU-DEBUGGER-ABI-1-0 %s
 // CHECK-MAMDGPU-DEBUGGER-ABI-1-0: "-target-feature" 
"+amdgpu-debugger-insert-nops" "-target-feature" 
"+amdgpu-debugger-emit-prologue"
 
+// RUN: %clang -### -target amdgcn -mcpu=gfx700 -mcode-object-v3 %s 2>&1 | 
FileCheck --check-prefix=CODE-OBJECT-V3 %s
+// CODE-OBJECT-V3: "-target-feature" "+code-object-v3"
+
+// RUN: %clang -### -target amdgcn -mcpu=gfx700 -mno-code-object-v3 %s 2>&1 | 
FileCheck --check-prefix=NO-CODE-OBJECT-V3 %s
+// NO-CODE-OBJECT-V3: "-target-feature" "-code-object-v3"
+
 // RUN: %clang -### -target amdgcn -mcpu=gfx700 -mxnack %s 2>&1 | FileCheck 
--check-prefix=XNACK %s
 // XNACK: "-target-feature" "+xnack"
 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from some minor nits.




Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:71
+void MacroUsageCheck::warnMacro(const MacroDirective *MD) {
+  auto MessageFactory = [](const MacroInfo *Info) {
+/// A variadic macro is function-like at the same time. Therefore variadic

It seems unnecessarily complicated, to me, to use a lambda here. Why not use a 
StringRef local variable?
```
StringRef Msg = "blah";
if (Info->isVariadic())
  Msg = "blah";
else if (Info->isFunctionLike())
  Msg = "blah";
```



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78
+ "variadic template function";
+else if (Info->isFunctionLike())
+  return "function-like macro used; consider a 'constexpr' template "

Nit: `else` after `return`



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:81
+ "function";
+else
+  return "macro used to declare a constant; consider using a 'constexpr' "

Nit: same



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:34-35
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void warnMacro(const MacroDirective *MD);
+  void warnNaming(const MacroDirective *MD);
+

Nit: these can be private functions instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from minor wording nits.




Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:31-32
+  static constexpr llvm::StringLiteral SkipFirst = llvm::StringLiteral("");
+  // Suffix can only consist of 'u' and 'l' chars, and can be a complex number.
+  // Also, in MS compatibility mode, suffixes like i32 are supported.
+  static constexpr llvm::StringLiteral Suffixes =

The comment is somewhat out of sync with the code because of the i and j 
suffixes as well. 



Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:45
+  // C++17 introduced hexadecimal floating-point literals, and 'f' is both
+  // 15 in decimal and is 'f' as in 'floating point suffix'.
+  // So we can't just "skip to the chars that can be in the suffix".

I think you mean f is both a valid hexadecimal digit in a hex float literal and 
a valid floating-point literal suffix.



Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:101
+return *NewSuffix;
+  // Nope, i guess we have to keep it as-is.
+  return llvm::None;

i -> I



Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:212
+
+  // We won't *always* want to diagnose. We might have already-uppercase 
suffix.
+  if (auto Details = shouldReplaceLiteralSuffix(

have already-uppercase suffix -> have a suffix that is already uppercase



Comment at: clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp:216
+auto Complaint = diag(Details->LiteralLocation,
+  "%0 literal has suffix '%1', which is not 
upper-case")
+ << LiteralType::Name << Details->OldSuffix;

upper-case -> uppercase



Comment at: docs/clang-tidy/checks/readability-uppercase-literal-suffix.rst:28
+When the suffix is found, a case-insensitive lookup in that list is made,
+and if replacement is found, and it is different from the current suffix,
+only then the diagnostic is issued.

I'd reword this slightly:

"and if a replacement is found that is different from the current suffix, then 
the diagnostic is issued. This allows for fine-grained control of what suffixes 
to consider and what their replacements should be."


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52670



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53387: [clangd] Lay JSONRPCDispatcher to rest.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous, 
MaskRay, javed.absar, ilya-biryukov, mgorny.

Most of its functionality is moved into ClangdLSPServer.
The decoupling between JSONRPCDispatcher, ProtocolCallbacks, ClangdLSPServer
was never real, and only served to obfuscate.

Some previous implicit/magic stuff is now explicit:

- the return type of LSP method calls are now in the signature
- no more reply() that gets the ID using global context magic
- arg tracing no longer relies on RequestArgs::stash context magic either

This is mostly refactoring, but some deliberate fixes while here:

- LSP method params are now by const reference
- notifications and calls are now distinct namespaces. (some tests had protocol 
errors and needed updating)
- we now reply to calls we failed to decode
- outgoing calls use distinct IDs

A few error codes and message IDs changed in unimportant ways (see tests).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53387

Files:
  clangd/CMakeLists.txt
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/JSONRPCDispatcher.cpp
  clangd/JSONRPCDispatcher.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/TUScheduler.cpp
  clangd/tool/ClangdMain.cpp
  test/clangd/crash-non-added-files.test
  test/clangd/delimited-input-comment-at-the-end.test
  test/clangd/fixits-command.test
  test/clangd/rename.test
  test/clangd/spaces-in-delimited-input.test

Index: test/clangd/spaces-in-delimited-input.test
===
--- test/clangd/spaces-in-delimited-input.test
+++ test/clangd/spaces-in-delimited-input.test
@@ -9,5 +9,5 @@
 
 ---
 
-{"jsonrpc":"2.0","id":3,"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
 # CHECK-NOT: JSON parse error
Index: test/clangd/rename.test
===
--- test/clangd/rename.test
+++ test/clangd/rename.test
@@ -28,7 +28,7 @@
 ---
 {"jsonrpc":"2.0","id":2,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #  CHECK:  "error": {
-# CHECK-NEXT:"code": -32603,
+# CHECK-NEXT:"code": -32001,
 # CHECK-NEXT:"message": "clang diagnostic"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
Index: test/clangd/fixits-command.test
===
--- test/clangd/fixits-command.test
+++ test/clangd/fixits-command.test
@@ -167,7 +167,7 @@
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": "Fix applied."
 #
-#  CHECK:  "id": 1,
+#  CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "method": "workspace/applyEdit",
 # CHECK-NEXT:  "params": {
Index: test/clangd/delimited-input-comment-at-the-end.test
===
--- test/clangd/delimited-input-comment-at-the-end.test
+++ test/clangd/delimited-input-comment-at-the-end.test
@@ -8,5 +8,4 @@
 ---
 {"jsonrpc":"2.0","id":3,"method":"shutdown"}
 ---
-{"jsonrpc":"2.0","id":3,"method":"exit"}
-# comment at the end
+{"jsonrpc":"2.0","method":"exit"}
Index: test/clangd/crash-non-added-files.test
===
--- test/clangd/crash-non-added-files.test
+++ test/clangd/crash-non-added-files.test
@@ -32,5 +32,3 @@
 {"jsonrpc":"2.0","id":6,"method":"shutdown"}
 ---
 {"jsonrpc":"2.0","method":"exit"}

-{"jsonrpc":"2.0","method":"exit"}
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -8,10 +8,9 @@
 //===--===//
 
 #include "ClangdLSPServer.h"
-#include "JSONRPCDispatcher.h"
 #include "Path.h"
-#include "RIFF.h"
 #include "Trace.h"
+#include "Transport.h"
 #include "index/Serialization.h"
 #include "clang/Basic/Version.h"
 #include "llvm/Support/CommandLine.h"
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -728,9 +728,8 @@
 llvm::unique_function)> Action) {
   auto It = Files.find(File);
   if (It == Files.end()) {
-Action(llvm::make_error(
-"trying to get AST for non-added document",
-llvm::errc::invalid_argument));
+Action(llvm::make_error(
+"trying to get AST for non-added document", ErrorCode::InvalidParams));
 return;
   }
 
@@ -742,9 +741,9 @@
 llvm::unique_function)> Action) {
   auto It = Files.find(File);
   if (It == Files.end()) {
-Action(llvm::make_error(
+Action(llvm::make_error(
 "trying to get preamble for non-added document",
-llvm::errc::invalid_argument));
+ErrorCode::InvalidParams));
 return;
   }
 
Index: clangd/ProtocolHandle

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52771#1263404, @lebedev.ri wrote:

> FIXME: `IgnoreClassesWithAllMemberVariablesBeingPublic` needs to be somehow 
> enabled for cppcoreguidelines check.
>  I don't know if it is possible, and how.


IIRC, the idea is to override `getModuleOptions()` and specify different 
default configuration options for the alias.




Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:82
+   "member variable '%0' of %1 '%2' has %3 visibility")
+  << Field->getName() << Record->getKindName() << Record->getName()
+  << Field->getAccess();

lebedev.ri wrote:
> aaron.ballman wrote:
> > Drop the single quotes above and just pass in `Field` and `Record` directly 
> > -- the diagnostic printer will do the right thing.
> Nice, but that does not seem to dump `getKindName()`.
Correct - it only works for automatically quoting `NamedDecl` objects



Comment at: clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp:90
+  diag(Field->getLocation(), "member variable %0 of %1 %2 has %3 visibility")
+  << Field << Record->getKindName() << Record << Field->getAccess();
+}

I cannot think of a situation in which the class name is actually useful 
information to present to the user, so I'd recommend dropping it (and the kind 
name as well). However, if you have a specific case in mind where the class 
name is useful for understanding how to fix the issue, I'd love to see it.



Comment at: 
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:9
+
+Finds classes that not only contain the data (non-static member variables),
+but also have logic (non-static member functions), and diagnoses all member

I'd reword this a bit to:
```
Finds classes that contain non-static data members in addition to non-static 
member functions and diagnose all data members declared with a non-``public`` 
access specifier. The data members should be declared as ``private`` and 
accessed through member functions instead of exposed to derived classes or 
class consumers.
```



Comment at: 
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:14-15
+
+Optionally, classes with all member variables being ``public`` could be
+ignored, and optionally all ``public`` member variables could be ignored.
+

I'd drop this paragraph entirely.



Comment at: 
docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst:28
+   Allows to ignore (not diagnose) **all** the member variables with ``public``
+   visibility scope.

with public visibility scope -> declared with a public access specifier


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52771



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52301: [clang] Set TypeSourceInfo for vardecl's in addition to type when we can deduce.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the context and explanation! I don't feel like I have enough 
familiarity with this machinery to be an effective reviewer for signing off on 
anything. I've commented with some nits, but @rsmith should give the sign-off 
on this one.




Comment at: include/clang/AST/PrettyPrinter.h:229
+
+  /// When true, prints deduced type for auto typed variables if their type has
+  /// been deduced. When false, always prints auto(or other variants e.g.

prints deduced -> prints the deduced



Comment at: include/clang/AST/PrettyPrinter.h:230
+  /// When true, prints deduced type for auto typed variables if their type has
+  /// been deduced. When false, always prints auto(or other variants e.g.
+  /// decltype(auto)) even if type was deduced.

Add a whitespace before the left paren.



Comment at: include/clang/Sema/Sema.h:7063
 
-  QualType deduceVarTypeFromInitializer(VarDecl *VDecl, DeclarationName Name,
-QualType Type, TypeSourceInfo *TSI,
-SourceRange Range, bool DirectInit,
-Expr *Init);
+  std::pair
+  deduceVarTypeFromInitializer(VarDecl *VDecl, DeclarationName Name,

I'd appreciate some comments as to why this is returning a pair and under what 
circumstances the type information will be different, because it's not likely 
to be obvious to anyone reading this header.



Comment at: lib/Frontend/ASTConsumers.cpp:92
 PrintingPolicy Policy(D->getASTContext().getLangOpts());
+// Since ASTPrinter is used for pretty printing and auto is generally
+// prettier than real type itself, we'll choose to print auto always.

Since -> Because

That said, I am not convinced I agree with this being the default. "Prettier" 
is pretty subjective, and I'd rather our default be for clarity instead of 
brevity, at least here.



Comment at: lib/Sema/SemaChecking.cpp:852
 
-  if (RT->getPointeeType().getAddressSpace() != LangAS::opencl_generic) {
-S.Diag(Call->getArg(0)->getBeginLoc(),

kadircet wrote:
> aaron.ballman wrote:
> > I'm not certain I understand why this code has been removed?
> It shouldn't have been, tried rebasing but it didn't go away. I think it was 
> deleted at some point by a different change and somehow ended up showing in 
> here as well. (Tried to revert, got an error stating 
> warn_opencl_generic_address_space_arg doesn't exist)
That's truly strange. I bring it up because these sort of changes make it 
harder for reviewers to test changes locally by applying the patch themselves.



Comment at: lib/Sema/SemaDecl.cpp:10912
  Expr *Init) {
-  QualType DeducedType = deduceVarTypeFromInitializer(
+  auto DeducedTypeAndTSI = deduceVarTypeFromInitializer(
   VDecl, VDecl->getDeclName(), VDecl->getType(), 
VDecl->getTypeSourceInfo(),

Don't use `auto` as the type is not spelled out in the initialization.



Comment at: lib/Sema/SemaStmt.cpp:1888
   if (First) {
-QualType FirstType;
+TypeSourceInfo* FirstTypeTSI;
 if (DeclStmt *DS = dyn_cast(First)) {

Formatting is off here (the `*` should bind to the right).



Comment at: lib/Sema/SemaStmt.cpp:1975
   // AddInitializerToDecl, so we can produce a more suitable diagnostic.
-  QualType InitType;
+  TypeSourceInfo* InitTypeTSI = nullptr;
   if ((!isa(Init) && Init->getType()->isVoidType()) ||

Formatting.



Comment at: lib/Sema/SemaStmt.cpp:3442
 //  argument deduction.
-DeduceAutoResult DAR = DeduceAutoType(OrigResultType, RetExpr, Deduced);
+TypeSourceInfo* DeducedTSI = nullptr;
+DeduceAutoResult DAR = DeduceAutoType(OrigResultType, RetExpr, DeducedTSI);

Same -- you should probably run the patch through clang-format 
(https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting).



Comment at: lib/Sema/SemaTemplateDeduction.cpp:4464
   .Apply(Type);
-  assert(!FuncParam.isNull() &&
+  assert(!FuncParamTSI->getType().isNull() &&
  "substituting template parameter for 'auto' failed");

Null check?


Repository:
  rC Clang

https://reviews.llvm.org/D52301



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53382: [clang-doc] Update documentation

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Will be good idea to mention improvements in Release Notes.




Comment at: clang-tools-extra/docs/clang-doc.rst:20
 Use
-=
+
 

Isn't it should be same length as heading? Same in other places.


https://reviews.llvm.org/D53382



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53025: [clang-tidy] implement new check for const return types.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/readability-const-value-return.cpp:174
+int **const * n_multiple_ptr();
+int *const & n_pointer_ref();

I'd like to see some more complex examples, like:
```
int const foo();
int const * const foo();

std::add_const foo();

template 
std::add_const foo();

auto foo() -> const int;
auto foo() -> int const;

template 
auto foo() -> std::add_const;
```
I'm also very curious to hear how often this triggers on large code bases, 
especially ones that claim decent const-correctness.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50403: [clang-format]AlignConsecutiveAssignments

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: include/clang/Format/Format.h:85
+  /// matching assignment operators. This includes consecutive |=, +=
+  /// -=, /=, *=. This will result in formattings like
   /// \code

Please add tests for these. Also it's not clear from these examples how would a 
block of lines using assigments spanning different number of columns would be 
alined, as in:
```
aaa = 1;
bb += 2;
c <<= 3;
```
vs.
```
aaa = 1;
bb  += 2;
c   <<= 3;
```

I think this might deserve discussion by itself before this patch can get in 
(personally, I'm in favor of the first version where the right hand sides are 
alined).


Repository:
  rC Clang

https://reviews.llvm.org/D50403



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

@nruslan This patchset is still pending review so be patient, I landed 
https://reviews.llvm.org/D53103 as it was reviewed and accepted by the code 
owner, on which this patch depends on. That said it LGTM, it's simple enough as 
it just forwards the argument to the backend, but still would rather have the 
X86 code owner sign off on this as well.


https://reviews.llvm.org/D53102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

[Craig Topper via Phabricator via cfe-commits]

craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

LGTM with that one style comment fixed.




Comment at: lib/Driver/ToolChains/Clang.cpp:1743
+  if (!Args.hasFlag(options::OPT_mtls_direct_seg_refs,
+  options::OPT_mno_tls_direct_seg_refs, true))
+CmdArgs.push_back("-mno-tls-direct-seg-refs");

minor - I think this line should be lined up with the start of the arguments to 
hasFlags on the previous line.


https://reviews.llvm.org/D53102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang-tools-extra] r344650 - [clang-doc] Add unit tests for serialization

[Mikael Holmén via cfe-commits]

On 10/17/2018 08:30 PM, Julie Hockett wrote:
> https://reviews.llvm.org/D53381 should fix this -- thanks for the note!

Yep, thanks!

/Mikael

> 
> Julie
> 
> On Wed, Oct 17, 2018 at 1:58 AM Mikael Holmén 
> mailto:mikael.hol...@ericsson.com>> wrote:
> 
> Hi Julie,
> 
> clang 3.6.0 complains on this commit:
> 
> /usr/bin/clang++  -march=corei7  -DGTEST_HAS_RTTI=0
> -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_LANG_CXX11=1 -D_DEBUG -D_GNU_SOURCE
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
> -Itools/clang/tools/extra/unittests/clang-doc
> -I../tools/clang/tools/extra/unittests/clang-doc
> -I../tools/clang/include -Itools/clang/include -I/usr/include/libxml2
> -Iinclude -I../include -I../tools/clang/tools/extra/clang-doc
> -I../utils/unittest/googletest/include
> -I../utils/unittest/googlemock/include
> -I/proj/flexasic/app/valgrind/3.11.0/include  -fPIC
> -fvisibility-inlines-hidden -Werror -Werror=date-time -std=c++11 -Wall
> -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual
> -Wmissing-field-initializers -pedantic -Wno-long-long
> -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor
> -Wstring-conversion -fdiagnostics-color -ffunction-sections
> -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types
> -O3    -UNDEBUG  -Wno-variadic-macros
> -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -MMD
> -MT
> 
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
> 
> -MF
> 
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o.d
> 
> -o
> 
> tools/clang/tools/extra/unittests/clang-doc/CMakeFiles/ClangDocTests.dir/BitcodeTest.cpp.o
> 
> -c ../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp
> In file included from
> ../tools/clang/tools/extra/unittests/clang-doc/BitcodeTest.cpp:12:
> ../tools/clang/tools/extra/unittests/clang-doc/ClangDocTest.h:25:14:
> error: suggest braces around initialization of subobject
> [-Werror,-Wmissing-braces]
>       SymbolID{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
> 1, 1};
>   
> ^~
>                {   
>       }
> 1 error generated.
> 
> Regards,
> Mikael
> 
> On 10/17/2018 01:06 AM, Julie Hockett via cfe-commits wrote:
>  > Author: juliehockett
>  > Date: Tue Oct 16 16:06:42 2018
>  > New Revision: 344650
>  >
>  > URL: http://llvm.org/viewvc/llvm-project?rev=344650&view=rev
>  > Log:
>  > [clang-doc] Add unit tests for serialization
>  >
>  > Adds unit tests for the Serialize library.
>  >
>  > This is part of a move to convert clang-doc's tests to a more
>  > maintainable unit test framework, with a smaller number of
> integration
>  > tests to maintain and more granular failure feedback.
>  >
>  > Differential Revision: https://reviews.llvm.org/D53081
>  >
>  > Added:
>  >      clang-tools-extra/trunk/unittests/clang-doc/
>  >      clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
>  >      clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.cpp
>  >      clang-tools-extra/trunk/unittests/clang-doc/ClangDocTest.h
>  >      clang-tools-extra/trunk/unittests/clang-doc/SerializeTest.cpp
>  > Modified:
>  >      clang-tools-extra/trunk/unittests/CMakeLists.txt
>  >
>  > Modified: clang-tools-extra/trunk/unittests/CMakeLists.txt
>  > URL:
> 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/CMakeLists.txt?rev=344650&r1=344649&r2=344650&view=diff
>  >
> 
> ==
>  > --- clang-tools-extra/trunk/unittests/CMakeLists.txt (original)
>  > +++ clang-tools-extra/trunk/unittests/CMakeLists.txt Tue Oct 16
> 16:06:42 2018
>  > @@ -16,6 +16,7 @@ endif()
>  >
>  >   add_subdirectory(change-namespace)
>  >   add_subdirectory(clang-apply-replacements)
>  > +add_subdirectory(clang-doc)
>  >   add_subdirectory(clang-move)
>  >   add_subdirectory(clang-query)
>  >   add_subdirectory(clang-tidy)
>  >
>  > Added: clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
>  > URL:
> 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt?rev=344650&view=auto
>  >
> 
> ==
>  > --- clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
> (added)
>  > +++ clang-tools-extra/trunk/unittests/clang-doc/CMakeLists.txt
> Tue Oct 16 16:06:42 2018
>  > @@ -0,0 +1,29 @@
>  > +set(LLVM_LINK_COMPONENTS
>  > +  support
> 

[clang-tools-extra] r344724 - Fix warning about unused variable [NFC]

[Mikael Holmen via cfe-commits]

Author: uabelho
Date: Wed Oct 17 23:00:39 2018
New Revision: 344724

URL: http://llvm.org/viewvc/llvm-project?rev=344724&view=rev
Log:
Fix warning about unused variable [NFC]

Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=344724&r1=344723&r2=344724&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Wed Oct 17 23:00:39 2018
@@ -562,7 +562,7 @@ getQueryScopes(CodeCompletionContext &CC
 for (auto *Context : CCContext.getVisitedContexts()) {
   if (isa(Context))
 Info.AccessibleScopes.push_back(""); // global namespace
-  else if (const auto *NS = dyn_cast(Context))
+  else if (isa(Context))
 Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
 }
 return Info;


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D52695



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53374: [clangd] Names that are not spelled in source code are reserved.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Idea looks good, I think it needs some renames and index support.




Comment at: clangd/AST.h:33
+// `-DName=foo`, the spelling location will be "".
+bool SpelledInSourceCode(const Decl *D);
+

nit: should start with lowercase: `isSpelledInSourceCode`?



Comment at: clangd/Quality.cpp:182
 if (auto *ID = SemaCCResult.Declaration->getIdentifier())
-  ReservedName = ReservedName || isReserved(ID->getName());
+  ReservedName = ReservedName || isReserved(ID->getName()) ||
+ !SpelledInSourceCode(SemaCCResult.Declaration);

This doesn't match the current definition of `ReservedName`.
I'd suggest either:
 - adding a new boolean (`ImplementationDetail`? maybe we'll add other 
heuristics) and treating this as independent of reserved-ness
 - renaming the current `ReservedName` flag to cover this expanded scope 
(again, `ImplementationDetail` is a reasonable name)



Comment at: clangd/Quality.cpp:192
   Category = categorize(IndexResult.SymInfo);
   ReservedName = ReservedName || isReserved(IndexResult.Name);
 }

The new `ReservedName` cases don't survive a round-trip through the index 
(unlike the existing ones, which get recomputed from the name).

I think you want to add a new flag bit to `Symbol`, set it in 
`SymbolCollector`, and read it here. (IIRC new flags in the `Flags` field are 
handled transparently by yaml and binary serialization).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53374



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53363: [clangd] Encode Line/Column as a 32-bits integer.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice, just nits!

In https://reviews.llvm.org/D53363#1267721, @hokein wrote:

> Yeah, I have a rough patch for it, using char* will save us ~50MB memory, 
> which will lead to ~300 MB memory usage in total.


For just the StringRef in SymbolLocation::Position, or for all our strings?
If the latter, that seems too low, as we should save strictly more than this 
patch (unless I'm missing something about alignment/padding)




Comment at: clangd/index/Index.h:41
   struct Position {
-uint32_t Line = 0; // 0-based
+static constexpr int LineBits = 20;
+static constexpr int ColumnBits = 12;

hokein wrote:
> sammccall wrote:
> > (not sure these constants are needed as it stands - YAML shouldn't use 
> > them, see below)
> I think we need, for testing, for setters, rather than using magic number.
In that case I'd suggest static constexpr `MaxLine = 1 << 20 - 1`, `MaxColumn = 
1 << 12 - 1`, as that's what the tests (and potentially some warning checks in 
the code later?) need, so we encapsulate a little more detail. But up to you.

(Writing 20 twice a couple of lines apart seems fine to me)



Comment at: clangd/index/Index.h:46
 // Using UTF-16 code units.
-uint32_t Column = 0; // 0-based
+uint32_t Column : ColumnBits; // 0-based
   };

hokein wrote:
> sammccall wrote:
> > If we overflow the columns, it would be much easier to recognize if the 
> > result is always e.g. 255, rather than an essentially random number from 
> > 0-255 (from modulo 256 arithmetic).
> > 
> > This would mean replacing the raw fields with setters/getters, which is 
> > obviously a more invasive change. What about a compromise: add the setters, 
> > and call them from the most important codepaths: `SymbolCollector` and 
> > `Serialization`.
> It seems reasonable to use maximum value if we encounter overflows.
> 
> > This would mean replacing the raw fields with setters/getters, which is 
> > obviously a more invasive change. What about a compromise: add the setters, 
> > and call them from the most important codepaths: SymbolCollector and 
> > Serialization.
> 
> Actually, there is not too much usage of the raw fields in the source code, 
> I'd prefer to use all setters/getters in all places (and it would make the 
> code consistent and more safe). How about this plan?
> 
> 1. I update the patch to change all raw fields usage to getters/setters, and 
> keep these raw fields public
> 2. do a cleanup internally
> 3. make these raw fields private
SGTM!



Comment at: clangd/index/Index.h:38
+  // Position is encoded into 32 bits to save space.
+  // If Line/Column is overflow, the value will be their maximum value.
   struct Position {

nit: no "is", overflow is a verb



Comment at: clangd/index/Index.h:41
+void setLine(uint32_t Line);
+uint32_t line() const;
+void setColumn(uint32_t Column);

nit: define the trivial getters here so the compiler can always inline them.
(Setters seem fine out-of-line)



Comment at: clangd/index/Index.h:66
const SymbolLocation::Position &R) {
-  return std::tie(L.Line, L.Column) == std::tie(R.Line, R.Column);
+  return std::make_tuple(L.line(), L.column()) ==
+ std::make_tuple(R.Line, R.Column);

Why only using the getters on one side?



Comment at: clangd/index/YAMLSerialization.cpp:98
 
-template <> struct MappingTraits {
-  static void mapping(IO &IO, SymbolLocation::Position &Value) {
-IO.mapRequired("Line", Value.Line);
-IO.mapRequired("Column", Value.Column);
+template <> struct MappingTraits> {
+  static void mapping(IO &IO, std::pair &Value) {

I don't think specializing the traits for `pair` is OK, 
specializing for common types we don't own is asking for ODR violations.

prefer to define an custom struct for this (in fact you do, so just map that 
struct instead of struct->P?)



Comment at: clangd/index/YAMLSerialization.cpp:104-105
 };
 
+struct NormalizedPosition {
+  using Position = clang::clangd::SymbolLocation::Position;

please document why this normalization (YamlIO can't directly map bitfields)



Comment at: clangd/index/YAMLSerialization.cpp:109
+  NormalizedPosition(IO &, const Position &Pos) {
+static_assert(
+sizeof(Position) == sizeof(uint32_t),

no need for this assert


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits