[PATCH] D50452: [WIP] clangd XPC adapter
jkorous created this revision.
jkorous added reviewers: arphaman, sammccall, ilya-biryukov, simark.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, ioeric, mgorny.
Based on our internal discussions we decided to change our direction with XPC
support for clangd. We did some extra measurements and found out that JSON
serialized over XPC is good enough for our use-case. We also took into
consideration that we'd have to deal with support for multiple workspaces in
near future.
Probably the simplest solution is to keep XPC completely out of clangd binary
and use one clangd instance per workspace. This design means it's the least
intrusive change, it's rather simple (compared to adding support for multiple
workspaces to clangd itself) and robust (compared to threads-based
implementation). Long-term it's hopefully also going to be less
maintenance-demanding since it's dependent only on LSP and not it's
implementation.
The patch is nearly finished - I just wanted to get some feedback on the design
early on (before finishing doxygen annotations, documentation, etc.).
Our approach to testing for the future is to reuse existing clangd lit tests
and just send messages through our XPC code. For the time being there's just a
single minimal testcase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50452
Files:
CMakeLists.txt
Features.inc.in
clangd/CMakeLists.txt
clangd/xpc/initialize.test
lit.cfg
lit.site.cfg.in
xpc/CMakeLists.txt
xpc/README.txt
xpc/cmake/Info.plist.in
xpc/cmake/XPCServiceInfo.plist.in
xpc/cmake/modules/CreateClangdXPCFramework.cmake
xpc/framework/CMakeLists.txt
xpc/framework/ClangdXPC.cpp
xpc/test-client/CMakeLists.txt
xpc/test-client/ClangdXPCTestClient.cpp
xpc/tool/CMakeLists.txt
xpc/tool/ClangdSubprocess.cpp
xpc/tool/ClangdSubprocess.h
xpc/tool/ClangdWorkspaceInstances.cpp
xpc/tool/ClangdWorkspaceInstances.h
xpc/tool/ClangdXpcAdapter.cpp
xpc/tool/log.h
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -51,4 +51,4 @@
clangToolingInclusions
LLVMSupport
LLVMTestingSupport
- )
+ )
\ No newline at end of file
Index: lit.site.cfg.in
===
--- lit.site.cfg.in
+++ lit.site.cfg.in
@@ -11,6 +11,7 @@
config.python_executable = "@PYTHON_EXECUTABLE@"
config.target_triple = "@TARGET_TRIPLE@"
config.clang_staticanalyzer = @CLANG_ENABLE_STATIC_ANALYZER@
+config.clangd_xpc_support = @CLANGD_BUILD_XPC@
# Support substitution of the tools and libs dirs with user parameters. This is
# used when we can't determine the tool dir at configuration time.
Index: lit.cfg
===
--- lit.cfg
+++ lit.cfg
@@ -117,6 +117,10 @@
if platform.system() not in ['Windows']:
config.available_features.add('ansi-escape-sequences')
+# XPC support for Clangd.
+if config.clangd_xpc_support:
+config.available_features.add('clangd-xpc-support')
+
if config.clang_staticanalyzer:
config.available_features.add('static-analyzer')
check_clang_tidy = os.path.join(
Index: clangd/xpc/initialize.test
===
--- /dev/null
+++ clangd/xpc/initialize.test
@@ -0,0 +1,10 @@
+# RUN: clangd-xpc-test-client < %s | FileCheck %s
+# REQUIRES: clangd-xpc-support
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+# CHECK-DAG: {"id":0,"jsonrpc":"2.0","result":{"capabilities":{"codeActionProvider":true,"completionProvider":{"resolveProvider":false,"triggerCharacters":[".",">",":"]},"definitionProvider":true,"documentFormattingProvider":true,"documentHighlightProvider":true,"documentOnTypeFormattingProvider":{"firstTriggerCharacter":"}","moreTriggerCharacter":[]},"documentRangeFormattingProvider":true,"documentSymbolProvider":true,"executeCommandProvider":{"commands":["clangd.applyFix"]},"hoverProvider":true,"renameProvider":true,"signatureHelpProvider":{"triggerCharacters":["(",","]},"textDocumentSync":2,"workspaceSymbolProvider":true}}}
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK-DAG: {"id":3,"jsonrpc":"2.0","result":null}
+
+{"jsonrpc":"2.0","method":"exit"}
Index: xpc/tool/log.h
===
--- /dev/null
+++ xpc/tool/log.h
@@ -0,0 +1,49 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_XPC_LOG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_XPC_LOG_H
+
+#include
+#include
+
+#include
+
+class Logger {
+private:
+ enum class log_type { log, info, debug };
+ log_type type;
+ std::string buffer_data;
+ std::stringstream buffer;
+ Logger(log_type type) : type(type), buffer_data(), buffer(buffer_data) {}
+
+public:
+ template Logger &operator<<(const T &in) {
+buffer << in;
+return *this;
+ }
+ friend
[PATCH] D49548: [clangd] XPC WIP
jkorous abandoned this revision. jkorous added a comment. We decided to abandon this direction in favor of simpler solution. https://reviews.llvm.org/D50452 One of the reasons is that there's a design fault - handling of server-originated messages (notifications) in this patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49548 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]
jkorous added a comment. Hi, we decided to go with a different solution: https://reviews.llvm.org/D50452 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48562: [clangd] XPC transport layer
jkorous abandoned this revision. jkorous added a comment. We decided to go with a different solution: https://reviews.llvm.org/D50452 https://reviews.llvm.org/D48562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous created this revision.
jkorous added reviewers: sammccall, ilya-biryukov.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, arphaman, dexonsmith, MaskRay, ioeric.
There's a small typo in tests - causing that we aren't sending exit LSP message
to clangd but crashing it instead with the last message not being valid JSON
and clangd hitting unexpected EOF in runLanguageServerLoop() right after. Seems
like this bug even managed to spawn some offsprings via copy-pasting. I just
found it by accident since I am not closing stdin after the last message when
using XPC adapter and my test was hanging - waiting for clangd to exit.
It's not a big deal but I got two ideas.
1. Maybe we should somehow check stderr from clangd - in this case there was
JSON parse error present but tests didn't notice.
2. When fixing that I got surprised by shutdown-without-exit.test
implementation. It seems more like exit-without-shutdown to me and makes sense
that we still want to exit (with non-success return value) in such case. Was
this the intention?
What do you think?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50641
Files:
clangd/extra-flags.test
clangd/formatting.test
clangd/initialize-params.test
clangd/shutdown-with-exit.test
clangd/shutdown-without-exit.test
clangd/unsupported-method.test
Index: clangd/unsupported-method.test
===
--- clangd/unsupported-method.test
+++ clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-without-exit.test
===
--- clangd/shutdown-without-exit.test
+++ clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-with-exit.test
===
--- clangd/shutdown-with-exit.test
+++ clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/initialize-params.test
===
--- clangd/initialize-params.test
+++ clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/formatting.test
===
--- clangd/formatting.test
+++ clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/extra-flags.test
===
--- clangd/extra-flags.test
+++ clangd/extra-flags.test
@@ -47,6 +47,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/unsupported-method.test
===
--- clangd/unsupported-method.test
+++ clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-without-exit.test
===
--- clangd/shutdown-without-exit.test
+++ clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/shutdown-with-exit.test
===
--- clangd/shutdown-with-exit.test
+++ clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/initialize-params.test
===
--- clangd/initialize-params.test
+++ clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/formatting.test
===
--- clangd/formatting.test
+++ clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/extra-flags.test
===
--- clangd/extra-flags.t
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous added a comment. I think that by introducing different codepath for testing purposes we might end up with fewer bugs in tests but the actual production code could become less tested. Actually, even the -lit-test itself might be not the theoretically most correct approach but I do see the practical reason for it. In general I'd rather keep the testing specific code to a minimum though. What we might be able to do specifically for this case is to return false iff we hit unexpected EOF in clangd::runLanguageServerLoop() (currently void) and || it with ShutdownRequestReceived in ClangdLSPServer::run(). I still see some value in monitoring clangd's stderr in tests in general though. I can imagine something simple like redirection of clangd's stderr to a file and just grepping the log for lines starting with "E[" and in case of some set of errors being expected/allowed for specific test case then grepping for negation (grep -v expected_error). There might be some work necessary to either recalibrate log levels or add allowed errors to large part of tests. For example clangd currently logs this error for most tests: E[11:24:49.910] Failed to get status for RootPath clangd: No such file or directory Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
jkorous added a comment. I can see the value of getting more information in case of unexpected test behaviour but I still don't really like to have separate codepath for testing purposes. Anyway, it's not a big deal and it looks like you guys are all in agreement about this. I created a patch that I will put for review (rebuilding and running tests overnight). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50641 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50641: [clangd][test] Fix exit messages in tests
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339781: [clangd][tests] Fix typo in tests - invalid LSP exit
message (authored by jkorous, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50641?vs=160367&id=160819#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50641
Files:
clang-tools-extra/trunk/test/clangd/extra-flags.test
clang-tools-extra/trunk/test/clangd/formatting.test
clang-tools-extra/trunk/test/clangd/initialize-params.test
clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
clang-tools-extra/trunk/test/clangd/unsupported-method.test
Index: clang-tools-extra/trunk/test/clangd/extra-flags.test
===
--- clang-tools-extra/trunk/test/clangd/extra-flags.test
+++ clang-tools-extra/trunk/test/clangd/extra-flags.test
@@ -49,6 +49,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/formatting.test
===
--- clang-tools-extra/trunk/test/clangd/formatting.test
+++ clang-tools-extra/trunk/test/clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/unsupported-method.test
===
--- clang-tools-extra/trunk/test/clangd/unsupported-method.test
+++ clang-tools-extra/trunk/test/clangd/unsupported-method.test
@@ -13,4 +13,4 @@
---
{"jsonrpc":"2.0","id":2,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-with-exit.test
@@ -1,4 +1,4 @@
# RUN: clangd -lit-test < %s
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/extra-flags.test
===
--- clang-tools-extra/trunk/test/clangd/extra-flags.test
+++ clang-tools-extra/trunk/test/clangd/extra-flags.test
@@ -49,6 +49,6 @@
---
{"jsonrpc":"2.0","id":5,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
===
--- clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
+++ clang-tools-extra/trunk/test/clangd/shutdown-without-exit.test
@@ -1,2 +1,2 @@
# RUN: not clangd -lit-test < %s
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/initialize-params.test
===
--- clang-tools-extra/trunk/test/clangd/initialize-params.test
+++ clang-tools-extra/trunk/test/clangd/initialize-params.test
@@ -46,4 +46,4 @@
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": null
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/formatting.test
===
--- clang-tools-extra/trunk/test/clangd/formatting.test
+++ clang-tools-extra/trunk/test/clangd/formatting.test
@@ -184,4 +184,4 @@
---
{"jsonrpc":"2.0","id":6,"method":"shutdown"}
---
-{"jsonrpc":"2.0":"method":"exit"}
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/trunk/test/clangd/unsupported-method.test
===
--- clang-tools-extra/trunk/test/clangd/unsupported-method.test
+++ clang-tools-extra/trunk/test/clangd/
[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode
jkorous created this revision.
jkorous added reviewers: arphaman, ilya-biryukov.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, ioeric.
This is meant to cause a visible fail when clangd gets invalid JSON payload in
-lit-test mode.
Based on discussion in https://reviews.llvm.org/D50641 (and
http://lists.llvm.org/pipermail/clangd-dev/2018-August/72.html).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50785
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
JSONRPCDispatcher.cpp
JSONRPCDispatcher.h
tool/ClangdMain.cpp
Index: tool/ClangdMain.cpp
===
--- tool/ClangdMain.cpp
+++ tool/ClangdMain.cpp
@@ -308,5 +308,6 @@
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
- return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
+ return LSPServer.run(stdin, Test, InputStyle) ? 0
+: NoShutdownRequestErrorCode;
}
Index: JSONRPCDispatcher.h
===
--- JSONRPCDispatcher.h
+++ JSONRPCDispatcher.h
@@ -109,7 +109,8 @@
/// with signals, which are sent by debuggers on some OSs.
void runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
JSONStreamStyle InputStyle,
- JSONRPCDispatcher &Dispatcher, bool &IsDone);
+ JSONRPCDispatcher &Dispatcher, bool &IsDone,
+ bool TestMode);
} // namespace clangd
} // namespace clang
Index: JSONRPCDispatcher.cpp
===
--- JSONRPCDispatcher.cpp
+++ JSONRPCDispatcher.cpp
@@ -342,8 +342,8 @@
// The C APIs seem to be clearer in this respect.
void clangd::runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
JSONStreamStyle InputStyle,
- JSONRPCDispatcher &Dispatcher,
- bool &IsDone) {
+ JSONRPCDispatcher &Dispatcher, bool &IsDone,
+ bool TestMode) {
auto &ReadMessage =
(InputStyle == Delimited) ? readDelimitedMessage : readStandardMessage;
while (!IsDone && !feof(In)) {
@@ -362,6 +362,8 @@
// Parse error. Log the raw message.
vlog("<<< {0}\n", *JSON);
elog("JSON parse error: {0}", llvm::toString(Doc.takeError()));
+if (TestMode)
+ exit(EXIT_FAILURE);
}
}
}
Index: ClangdLSPServer.h
===
--- ClangdLSPServer.h
+++ ClangdLSPServer.h
@@ -43,7 +43,7 @@
/// each instance of ClangdLSPServer.
///
/// \return Whether we received a 'shutdown' request before an 'exit'
request.
- bool run(std::FILE *In,
+ bool run(std::FILE *In, bool TestMode,
JSONStreamStyle InputStyle = JSONStreamStyle::Standard);
private:
Index: ClangdLSPServer.cpp
===
--- ClangdLSPServer.cpp
+++ ClangdLSPServer.cpp
@@ -447,7 +447,8 @@
CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
Server(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this, Opts) {}
-bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
+bool ClangdLSPServer::run(std::FILE *In, bool TestMode,
+ JSONStreamStyle InputStyle) {
assert(!IsDone && "Run was called before");
// Set up JSONRPCDispatcher.
@@ -457,7 +458,7 @@
registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
// Run the Language Server loop.
- runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
+ runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone, TestMode);
// Make sure IsDone is set to true after this method exits to ensure
assertion
// at the start of the method fires if it's ever executed again.
Index: tool/ClangdMain.cpp
===
--- tool/ClangdMain.cpp
+++ tool/ClangdMain.cpp
@@ -308,5 +308,6 @@
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
- return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
+ return LSPServer.run(stdin, Test, InputStyle) ? 0
+: NoShutdownRequestErrorCode;
}
Index: JSONRPCDispatcher.h
===
--- JSONRPCDispatcher.h
+++ JSONRPCDispatcher.h
@@ -109,7 +109,8 @@
/// with signals, which are sent by debuggers on some OSs.
void runLanguageServerLoop(std::FILE *In, JSONOutput &Out,
JSONStreamStyle InputStyle,
- JSO
[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected
jkorous added a reviewer: vsapsai. jkorous added a comment. Adding Volodymyr who landed related patch recently. Repository: rC Clang https://reviews.llvm.org/D50462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness
jkorous added a comment. Hi Alex, nice work! I am just wondering if it would be beneficial to assert Loc and End are in the same file. It might help to catch bugs. I also stumbled upon this function but not sure it makes things significantly cleaner here: https://clang.llvm.org/doxygen/SourceLocation_8h_source.html#l00175 LGTM otherwise. Repository: rC Clang https://reviews.llvm.org/D50740 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode
jkorous added a comment. Oh, I thought that what everyone wanted was test-specific behaviour. I like both approaches you propose much more! If we go for the generic option we can effectively start "checking stderr" in tests by using the flag. That would be nice. Just to be sure we are all on the same page: 1. Do we want to exit right after elog() call? 2. Do we consider calling exit() in elog() smart or hacky? Based on quick grepping if we'd like to propagate errors probably the biggest challenge would be ASTWorker. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous created this revision.
jkorous added reviewers: arphaman, erik.pilkington, vsapsai.
jkorous added a project: clang.
Herald added a subscriber: cfe-commits.
E. g. use "10.11" instead of "10_11".
This is just a consistency fix - the dotted format is already used everywhere
else.
rdar://problem/39845032
Repository:
rC Clang
https://reviews.llvm.org/D46747
Files:
lib/Sema/SemaDeclAttr.cpp
test/FixIt/fixit-availability-method.mm
Index: test/FixIt/fixit-availability-method.mm
===
--- /dev/null
+++ test/FixIt/fixit-availability-method.mm
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability
-fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+@interface foo
+- (void) method_bar __attribute__((availability(macos, introduced = 10.12)));
// expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+// expected-warning {{'method_bar' is only available on macOS 10.12 or
newer}} \
+// expected-note {{enclose 'method_bar' in an @available check to silence
this warning}} \
+// CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+return 0;
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -7467,6 +7467,7 @@
const AvailabilityAttr *AA =
getAttrForPlatform(SemaRef.getASTContext(), OffendingDecl);
VersionTuple Introduced = AA->getIntroduced();
+Introduced.UseDotAsSeparator();
if (AvailabilityStack.back() >= Introduced)
return;
Index: test/FixIt/fixit-availability-method.mm
===
--- /dev/null
+++ test/FixIt/fixit-availability-method.mm
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability -fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+@interface foo
+- (void) method_bar __attribute__((availability(macos, introduced = 10.12))); // expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+// expected-warning {{'method_bar' is only available on macOS 10.12 or newer}} \
+// expected-note {{enclose 'method_bar' in an @available check to silence this warning}} \
+// CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+return 0;
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -7467,6 +7467,7 @@
const AvailabilityAttr *AA =
getAttrForPlatform(SemaRef.getASTContext(), OffendingDecl);
VersionTuple Introduced = AA->getIntroduced();
+Introduced.UseDotAsSeparator();
if (AvailabilityStack.back() >= Introduced)
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous planned changes to this revision.
jkorous added a comment.
Sorry, my bad. I tried to get rid of dependency on Foundation.h and didn't
check the test is relevant for the fix after that.
#import
@interface foo
- (void) method NS_AVAILABLE_MAC(10_12);
@end
int main() {
[foo method];
return 0;
}
Looks like NS_AVAILABLE_MAC is not equivalent to the attribute.
Interesting question about UsesUnderscores, thanks! It looks like it must be
set through constructor somewhere - will figure this out.
Repository:
rC Clang
https://reviews.llvm.org/D46747
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous updated this revision to Diff 146605.
jkorous added a comment.
Fixed the test. It turned out that version component separator for availability
is set during parsing which is why all other tests (including my bad one) are
passing.
The only way how to set underscore as a separator is through VersionTuple
constructor...
https://clang.llvm.org/doxygen/classclang_1_1VersionTuple.html#a71e5354ccb617494136bdcb4d208a573
clang::VersionTuple::VersionTuple ( unsignedMajor,
unsignedMinor,
unsignedSubminor,
unsignedBuild,
boolUsesUnderscores = false
)
...which is exactly what is happening during parsing version tuple...
VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
// ...
return VersionTuple(Major, Minor, Subminor, (AfterMajorSeparator == '_'));
}
...which function is called when parsing availability attribute:
void Parser::ParseAvailabilityAttribute(IdentifierInfo &Availability,
// ...
VersionTuple Version = ParseVersionTuple(VersionRange);
// ...
}
https://reviews.llvm.org/D46747
Files:
lib/Sema/SemaDeclAttr.cpp
test/FixIt/fixit-availability-method.mm
Index: test/FixIt/fixit-availability-method.mm
===
--- /dev/null
+++ test/FixIt/fixit-availability-method.mm
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability
-fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+@interface foo
+- (void) method_bar __attribute__((availability(macosx, introduced = 10_12)));
// expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+// expected-warning {{'method_bar' is only available on macOS 10.12 or
newer}} \
+// expected-note {{enclose 'method_bar' in an @available check to silence
this warning}} \
+// CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+return 0;
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -7467,6 +7467,7 @@
const AvailabilityAttr *AA =
getAttrForPlatform(SemaRef.getASTContext(), OffendingDecl);
VersionTuple Introduced = AA->getIntroduced();
+Introduced.UseDotAsSeparator();
if (AvailabilityStack.back() >= Introduced)
return;
Index: test/FixIt/fixit-availability-method.mm
===
--- /dev/null
+++ test/FixIt/fixit-availability-method.mm
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability -fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+@interface foo
+- (void) method_bar __attribute__((availability(macosx, introduced = 10_12))); // expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+// expected-warning {{'method_bar' is only available on macOS 10.12 or newer}} \
+// expected-note {{enclose 'method_bar' in an @available check to silence this warning}} \
+// CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+return 0;
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -7467,6 +7467,7 @@
const AvailabilityAttr *AA =
getAttrForPlatform(SemaRef.getASTContext(), OffendingDecl);
VersionTuple Introduced = AA->getIntroduced();
+Introduced.UseDotAsSeparator();
if (AvailabilityStack.back() >= Introduced)
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous added a comment. I am not 100% sure it's the best thing to set printing style at the point of parsing version tuples but I am not sure it's bad either. Unless someone convinces me otherwise I would rather not do any major changes. https://reviews.llvm.org/D46747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal
jkorous created this revision.
jkorous added reviewers: arphaman, dexonsmith, rsmith, doug.gregor.
jkorous added a project: clang.
Herald added a subscriber: cfe-commits.
C++17 adds form of static_assert without string literal.
static_assert ( bool_constexpr )
Currently clang produces diagnostics like this:
assert.cpp:1:1: error: static_assert failed
static_assert(false);
^ ~
1 error generated.
This patch adds assert expression to error message in case string literal is
missing:
/Users/jkorous/assert.cpp:1:1: error: static_assert failed "false"
static_assert(false);
^ ~
1 error generated.
Reasons why printing assert expression in absence of string literal is useful
are:
1. Serialized diagnostics (--serialize-diagnostics) don't contain code snippet
and thus error message lack context.
2. No all tools scraping clang diagnostics use caret snippet provided by
-f-caret-diagnostics.
This patch doesn't address message length limit as currently clang doesn't
truncate messages anyway but it might be subject of another patch.
rdar://problem/38931241
Repository:
rC Clang
https://reviews.llvm.org/D46834
Files:
lib/Parse/ParseDeclCXX.cpp
lib/Sema/SemaDeclCXX.cpp
test/SemaCXX/static-assert-cxx17.cpp
test/SemaCXX/static-assert.cpp
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -50,7 +50,7 @@
StaticAssertProtected sap2; // expected-note {{instantiation}}
static_assert(true); // expected-warning {{C++17 extension}}
-static_assert(false); // expected-error-re {{failed{{$ expected-warning
{{extension}}
+static_assert(false); // expected-error {{static_assert failed "false"}}
expected-warning {{C++17 extension}}
// Diagnostics for static_assert with multiple conditions
Index: test/SemaCXX/static-assert-cxx17.cpp
===
--- /dev/null
+++ test/SemaCXX/static-assert-cxx17.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+static_assert(false, "foo"); // expected-error {{static_assert failed "foo"}}
+static_assert(false == true); // expected-error {{static_assert failed "false
== true"}}
+static_assert(false, "foo \"bar\" baz"); // expected-error {{static_assert
failed "foo \"bar\" baz"}}
Index: lib/Sema/SemaDeclCXX.cpp
===
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -13736,9 +13736,16 @@
if (!Failed && !Cond) {
SmallString<256> MsgBuffer;
- llvm::raw_svector_ostream Msg(MsgBuffer);
- if (AssertMessage)
-AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+ {
+llvm::raw_svector_ostream Msg(MsgBuffer);
+if (AssertMessage)
+ AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+else {
+ AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
+ MsgBuffer.insert(MsgBuffer.begin(), '\"');
+ MsgBuffer.append(1, '\"');
+}
+ }
Expr *InnerCond = nullptr;
std::string InnerCondDescription;
@@ -13747,11 +13754,12 @@
/*AllowTopLevelCond=*/false);
if (InnerCond) {
Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
- << InnerCondDescription << !AssertMessage
- << Msg.str() << InnerCond->getSourceRange();
+ << InnerCondDescription << MsgBuffer.empty() << MsgBuffer.str()
+ << InnerCond->getSourceRange();
} else {
Diag(StaticAssertLoc, diag::err_static_assert_failed)
- << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
+ << MsgBuffer.empty() << MsgBuffer.str()
+ << AssertExpr->getSourceRange();
}
Failed = true;
}
Index: lib/Parse/ParseDeclCXX.cpp
===
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -819,7 +819,10 @@
/// ParseStaticAssertDeclaration - Parse C++0x or C11
static_assert-declaration.
///
-/// [C++0x] static_assert-declaration:
+/// [C++17] static_assert-declaration:
+/// static_assert ( constant-expression ) ;
+///
+/// [C++11] static_assert-declaration:
/// static_assert ( constant-expression , string-literal ) ;
///
/// [C11] static_assert-declaration:
Index: test/SemaCXX/static-assert.cpp
===
--- test/SemaCXX/static-assert.cpp
+++ test/SemaCXX/static-assert.cpp
@@ -50,7 +50,7 @@
StaticAssertProtected sap2; // expected-note {{instantiation}}
static_assert(true); // expected-warning {{C++17 extension}}
-static_assert(false); // expected-error-re {{failed{{$ expected-warning {{extension}}
+static_assert(false); // expected-error {{stati
[PATCH] D46485: Add python tool to dump and construct header maps
jkorous added a comment.
Hi Bruno, I just noticed couple of implementation details.
Comment at: utils/hmaptool/hmaptool:55
+# The number of buckets must be a power of two.
+if num_buckets == 0 or (num_buckets & num_buckets - 1) != 0:
+raise SystemExit("error: %s: invalid number of buckets" % (
Wouldn't it be simpler to use modulo 2?
```
if num_buckets % 2 == 0
```
Comment at: utils/hmaptool/hmaptool:153
+def next_power_of_two(value):
+for i in range(32):
+power = 1 << i
This is probably not critically important but maybe we could use math functions
instead of iteration here:
```
from math import *
return ceil( log( value + 1, 2) )
```
...or if we want to support values <1:
```
if value <= 0
raise ArgumentError
if value < 1
return 0
return ceil( log( value, 2) ) + 1
```
Comment at: utils/hmaptool/hmaptool:234
+
+if len(args) != 2:
+parser.error("invalid number of arguments")
Aren't we expecting just single argument () here?
Repository:
rC Clang
https://reviews.llvm.org/D46485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous planned changes to this revision. jkorous added a comment. Sorry for me being dense here - since the output format is determined by input source code there's more work to do. I tried to change format of introduced version in couple of tests and the output mirrors the change. That basically means that whenever someone uses underscore __attribute__((availability(macosx, introduced = 10_12))) instead of dot __attribute__((availability(macosx, introduced = 10.12))) Warnings and FixIts get underscore as well. I am now thinking about just switching to dot format in ParseAvailabilityAttribute() because it's much simpler to maintain consistency that way. https://reviews.llvm.org/D46747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous added a comment. Eric, thanks for the context. I am now clarifying internally and if possible would like to simplify the whole format business by using just one delimiter everywhere. Would that make sense to you or do you think we should respect delimiter used in sources? https://reviews.llvm.org/D46747 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal
jkorous abandoned this revision. jkorous added a comment. We reconsidered this in light of the policy - thanks for pointing that out Richard! Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right? Repository: rC Clang https://reviews.llvm.org/D46834 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46485: Add python tool to dump and construct header maps
jkorous added a comment.
LGTM but my review was fairly superficial.
Comment at: utils/hmaptool/hmaptool:55
+# The number of buckets must be a power of two.
+if num_buckets == 0 or (num_buckets & num_buckets - 1) != 0:
+raise SystemExit("error: %s: invalid number of buckets" % (
bruno wrote:
> jkorous wrote:
> > Wouldn't it be simpler to use modulo 2?
> >
> > ```
> > if num_buckets % 2 == 0
> > ```
> We want it to be a power of two, not just a multiple.
Of course! Sorry, my mistake.
Comment at: utils/hmaptool/hmaptool:155
+raise ArgumentError
+return 1 if value == 0 else 2**(value - 1).bit_length()
+
Thanks for teaching me bit_length()!
https://reviews.llvm.org/D46485
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
jkorous updated this revision to Diff 147184.
jkorous added a comment.
This revision is now accepted and ready to land.
After some internal discussion we agreed that we can simplify things and get
consistent behaviour by using dot everywhere in diagnostics.
This is pretty much revert of r219124 + update of tests.
https://reviews.llvm.org/D46747
Files:
AST/DeclBase.cpp
Basic/VersionTuple.cpp
Misc/ast-print-objectivec.m
Parse/ParseDecl.cpp
Sema/availability-guard-format.mm
SemaObjC/attr-availability-1.m
clang/Basic/VersionTuple.h
Index: SemaObjC/attr-availability-1.m
===
--- SemaObjC/attr-availability-1.m
+++ SemaObjC/attr-availability-1.m
@@ -25,10 +25,10 @@
// rdar://11475360
@interface B : A
- (void)method; // NOTE: we expect 'method' to *not* inherit availability.
-- (void)overridden __attribute__((availability(macosx,introduced=10_4))); // expected-warning{{overriding method introduced after overridden method on macOS (10_4 vs. 10_3)}}
+- (void)overridden __attribute__((availability(macosx,introduced=10_4))); // expected-warning{{overriding method introduced after overridden method on macOS (10.4 vs. 10.3)}}
- (void)overridden2 __attribute__((availability(macosx,introduced=10_2)));
- (void)overridden3 __attribute__((availability(macosx,deprecated=10_4)));
-- (void)overridden4 __attribute__((availability(macosx,deprecated=10_2))); // expected-warning{{overriding method deprecated before overridden method on macOS (10_3 vs. 10_2)}}
+- (void)overridden4 __attribute__((availability(macosx,deprecated=10_2))); // expected-warning{{overriding method deprecated before overridden method on macOS (10.3 vs. 10.2)}}
- (void)overridden5 __attribute__((availability(macosx,introduced=10_3)));
- (void)overridden6 __attribute__((availability(macosx,unavailable))); // expected-warning{{overriding method cannot be unavailable on macOS when its overridden method is available}}
@end
Index: Sema/availability-guard-format.mm
===
--- /dev/null
+++ Sema/availability-guard-format.mm
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability -fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+// Testing that even for source code using '_' as a delimiter in availability version tuple '.' is actually used in diagnostic output as a delimiter.
+
+@interface foo
+- (void) method_bar __attribute__((availability(macosx, introduced = 10_12))); // expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+// expected-warning {{'method_bar' is only available on macOS 10.12 or newer}} \
+// expected-note {{enclose 'method_bar' in an @available check to silence this warning}} \
+// CHECK: "fix-it:.*if (@available(macOS 10.12, *))"
+return 0;
+}
Index: Misc/ast-print-objectivec.m
===
--- Misc/ast-print-objectivec.m
+++ Misc/ast-print-objectivec.m
@@ -30,7 +30,7 @@
// CHECK: @end
// CHECK: @interface I(CAT)
-// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10_1_0, deprecated=10_2)));
+// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2)));
// CHECK: @end
// CHECK: @implementation I
Index: Parse/ParseDecl.cpp
===
--- Parse/ParseDecl.cpp
+++ Parse/ParseDecl.cpp
@@ -762,8 +762,10 @@
///
/// version:
/// simple-integer
-/// simple-integer ',' simple-integer
-/// simple-integer ',' simple-integer ',' simple-integer
+/// simple-integer '.' simple-integer
+/// simple-integer '_' simple-integer
+/// simple-integer '.' simple-integer '.' simple-integer
+/// simple-integer '_' simple-integer '_' simple-integer
VersionTuple Parser::ParseVersionTuple(SourceRange &Range) {
Range = SourceRange(Tok.getLocation(), Tok.getEndLoc());
@@ -841,7 +843,7 @@
return VersionTuple();
}
-return VersionTuple(Major, Minor, (AfterMajorSeparator == '_'));
+return VersionTuple(Major, Minor);
}
const char AfterMinorSeparator = ThisTokBegin[AfterMinor];
@@ -872,7 +874,7 @@
return VersionTuple();
}
ConsumeToken();
- return VersionTuple(Major, Minor, Subminor, (AfterMajorSeparator == '_'));
+ return VersionTuple(Major, Minor, Subminor);
}
/// Parse the contents of the "availability" attribute.
Index: Basic/VersionTuple.cpp
===
--- Basic/VersionTuple.cpp
+++ Basic/VersionTuple.cpp
@@ -29,11 +29,11 @@
const VersionTuple &V) {
Out << V.getMajor();
if (Optional Minor = V.getMinor())
-Out << (V.usesUnderscores() ? '_' : '.') << *Minor;
+Out << '.' << *Minor;
if (Optional Subminor = V.getSubminor())
-Out << (V.usesUndersc
[PATCH] D46747: [Sema] Use dotted form of macOS version for unguarded availability FixIts
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332598: Use dotted format of version tuple for availability
diagnostics (authored by jkorous, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D46747?vs=147184&id=147295#toc
Repository:
rL LLVM
https://reviews.llvm.org/D46747
Files:
cfe/trunk/include/clang/Basic/VersionTuple.h
cfe/trunk/lib/AST/DeclBase.cpp
cfe/trunk/lib/Basic/VersionTuple.cpp
cfe/trunk/lib/Parse/ParseDecl.cpp
cfe/trunk/test/Misc/ast-print-objectivec.m
cfe/trunk/test/Sema/availability-guard-format.mm
cfe/trunk/test/SemaObjC/attr-availability-1.m
Index: cfe/trunk/include/clang/Basic/VersionTuple.h
===
--- cfe/trunk/include/clang/Basic/VersionTuple.h
+++ cfe/trunk/include/clang/Basic/VersionTuple.h
@@ -24,9 +24,7 @@
/// Represents a version number in the form major[.minor[.subminor[.build]]].
class VersionTuple {
- unsigned Major : 31;
-
- unsigned UsesUnderscores : 1;
+ unsigned Major : 32;
unsigned Minor : 31;
unsigned HasMinor : 1;
@@ -39,30 +37,25 @@
public:
VersionTuple()
- : Major(0), UsesUnderscores(false), Minor(0), HasMinor(false),
-Subminor(0), HasSubminor(false), Build(0), HasBuild(false) {}
+ : Major(0), Minor(0), HasMinor(false), Subminor(0), HasSubminor(false),
+Build(0), HasBuild(false) {}
explicit VersionTuple(unsigned Major)
- : Major(Major), UsesUnderscores(false), Minor(0), HasMinor(false),
-Subminor(0), HasSubminor(false), Build(0), HasBuild(false) {}
-
- explicit VersionTuple(unsigned Major, unsigned Minor,
-bool UsesUnderscores = false)
- : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-HasMinor(true), Subminor(0), HasSubminor(false), Build(0),
-HasBuild(false) {}
+ : Major(Major), Minor(0), HasMinor(false), Subminor(0),
+HasSubminor(false), Build(0), HasBuild(false) {}
- explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor,
-bool UsesUnderscores = false)
- : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-HasMinor(true), Subminor(Subminor), HasSubminor(true), Build(0),
-HasBuild(false) {}
+ explicit VersionTuple(unsigned Major, unsigned Minor)
+ : Major(Major), Minor(Minor), HasMinor(true), Subminor(0),
+HasSubminor(false), Build(0), HasBuild(false) {}
+
+ explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor)
+ : Major(Major), Minor(Minor), HasMinor(true), Subminor(Subminor),
+HasSubminor(true), Build(0), HasBuild(false) {}
explicit VersionTuple(unsigned Major, unsigned Minor, unsigned Subminor,
-unsigned Build, bool UsesUnderscores = false)
- : Major(Major), UsesUnderscores(UsesUnderscores), Minor(Minor),
-HasMinor(true), Subminor(Subminor), HasSubminor(true), Build(Build),
-HasBuild(true) {}
+unsigned Build)
+ : Major(Major), Minor(Minor), HasMinor(true), Subminor(Subminor),
+HasSubminor(true), Build(Build), HasBuild(true) {}
/// Determine whether this version information is empty
/// (e.g., all version components are zero).
@@ -93,14 +86,6 @@
return None;
return Build;
}
-
- bool usesUnderscores() const {
-return UsesUnderscores;
- }
-
- void UseDotAsSeparator() {
-UsesUnderscores = false;
- }
/// Determine if two version numbers are equivalent. If not
/// provided, minor and subminor version numbers are considered to be zero.
Index: cfe/trunk/test/Misc/ast-print-objectivec.m
===
--- cfe/trunk/test/Misc/ast-print-objectivec.m
+++ cfe/trunk/test/Misc/ast-print-objectivec.m
@@ -30,7 +30,7 @@
// CHECK: @end
// CHECK: @interface I(CAT)
-// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10_1_0, deprecated=10_2)));
+// CHECK: - (void)MethCAT __attribute__((availability(macos, introduced=10.1.0, deprecated=10.2)));
// CHECK: @end
// CHECK: @implementation I
Index: cfe/trunk/test/Sema/availability-guard-format.mm
===
--- cfe/trunk/test/Sema/availability-guard-format.mm
+++ cfe/trunk/test/Sema/availability-guard-format.mm
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -triple x86_64-apple-macosx-10.11 -Wunguarded-availability -fdiagnostics-parseable-fixits -fsyntax-only -verify %s
+
+// Testing that even for source code using '_' as a delimiter in availability version tuple '.' is actually used in diagnostic output as a delimiter.
+
+@interface foo
+- (void) method_bar __attribute__((availability(macosx, introduced = 10_12))); // expected-note {{'method_bar' has been explicitly marked partial here}}
+@end
+
+int main() {
+[foo method_bar]; // \
+
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple created this revision.
Added array type mangling to USR generation. Included test from bug report.
Repository:
rL LLVM
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep
"function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,22 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ VisitType(AT->getElementType());
+ Out << "[";
+
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+default : ;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T))
{
+Out << CAT->getSize();
+ }
+
+ Out << "]";
+ return;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep "function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,22 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ VisitType(AT->getElementType());
+ Out << "[";
+
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+default : ;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T)) {
+Out << CAT->getSize();
+ }
+
+ Out << "]";
+ return;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple updated this revision to Diff 118066.
jkorous-apple added a comment.
Uploaded full diff.
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep
"function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,22 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ VisitType(AT->getElementType());
+ Out << "[";
+
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+default : ;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T))
{
+Out << CAT->getSize();
+ }
+
+ Out << "]";
+ return;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep "function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,22 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ VisitType(AT->getElementType());
+ Out << "[";
+
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+default : ;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T)) {
+Out << CAT->getSize();
+ }
+
+ Out << "]";
+ return;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple updated this revision to Diff 118104.
jkorous-apple added a comment.
Ammenended as suggested.
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep
"function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,20 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+case ArrayType::Normal : Out << "n"; break;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T))
{
+Out << CAT->getSize();
+ }
+ Out << "}";
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,8 @@
+// RUN: c-index-test core -print-source-symbols -- %s | grep "function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+template<> void foo(char[16]);
+template<> void foo(char[32]);
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,20 @@
T = VT->getElementType();
continue;
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+case ArrayType::Normal : Out << "n"; break;
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T)) {
+Out << CAT->getSize();
+ }
+ Out << "}";
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple added a comment.
Replied and going to delete the end delimiter.
Comment at: lib/Index/USRGeneration.cpp:819
}
+if (const ArrayType *const AT = dyn_cast(T)) {
+ Out << "{";
arphaman wrote:
> You can use `const auto *` here.
That's what I wanted to do in the first place but decided against it because of
consistency - see other if conditions around. Do you still advice to use auto?
Comment at: lib/Index/USRGeneration.cpp:826
+ }
+ if (const ConstantArrayType* const CAT = dyn_cast(T))
{
+Out << CAT->getSize();
arphaman wrote:
> Ditto. Also, the braces are redundant.
Braces might be redundant yet I use them intentionally even for such cases
(think the infamous goto bug). But I can definitely delete those if you insist.
BTW Is there some consensus about this?
Comment at: lib/Index/USRGeneration.cpp:829
+ }
+ Out << "}";
+ T = AT->getElementType();
arphaman wrote:
> I don't think we need the terminating character.
I agree, going to delete that.
Comment at: test/Index/USR/array-type.cpp:1
+// RUN: c-index-test core -print-source-symbols -- %s | grep
"function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+
arphaman wrote:
> Please use `FileCheck` and verify the exact USR strings. This way you'll know
> that they're unique without actually trying to verify if they're unique in
> the test.
Since the USR format doesn't seem to be really stable I wanted to avoid
hard-coding USR values in tests. Wouldn't those tests be unnecessary brittle in
the sense that hard-coded values would have to be modified in case of future
changes?
https://reviews.llvm.org/D38643
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple updated this revision to Diff 118115.
jkorous-apple added a comment.
Solved issues from CR.
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# |
__Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# |
__Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# |
__Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,19 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+case ArrayType::Normal : Out << "n"; break;
+ }
+ if (const auto* const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# | __Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# | __Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# | __Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,19 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch( AT->getSizeModifier() ) {
+case ArrayType::Static : Out << "s"; break;
+case ArrayType::Star: Out << "*"; break;
+case ArrayType::Normal : Out << "n"; break;
+ }
+ if (const auto* const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple updated this revision to Diff 118231.
jkorous-apple added a comment.
clang-format
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# |
__Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# |
__Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# |
__Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,25 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch (AT->getSizeModifier()) {
+ case ArrayType::Static:
+Out << "s";
+break;
+ case ArrayType::Star:
+Out << "*";
+break;
+ case ArrayType::Normal:
+Out << "n";
+break;
+ }
+ if (const auto *const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# | __Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# | __Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# | __Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,25 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << "{";
+ switch (AT->getSizeModifier()) {
+ case ArrayType::Static:
+Out << "s";
+break;
+ case ArrayType::Star:
+Out << "*";
+break;
+ case ArrayType::Normal:
+Out << "n";
+break;
+ }
+ if (const auto *const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.
jkorous-apple updated this revision to Diff 118247.
jkorous-apple added a comment.
Single char constants don't need to be c-strings.
https://reviews.llvm.org/D38643
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/array-type.cpp
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should
have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# |
__Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# |
__Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# |
__Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,25 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << '{';
+ switch (AT->getSizeModifier()) {
+ case ArrayType::Static:
+Out << 's';
+break;
+ case ArrayType::Star:
+Out << '*';
+break;
+ case ArrayType::Normal:
+Out << 'n';
+break;
+ }
+ if (const auto *const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
Index: test/Index/USR/array-type.cpp
===
--- /dev/null
+++ test/Index/USR/array-type.cpp
@@ -0,0 +1,11 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Function template specializations differing in array type parameter should have unique USRs.
+
+template void foo(buffer);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n16C>#*C# | __Z3fooIA16_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[16]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n32C>#*C# | __Z3fooIA32_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[32]);
+// CHECK: {{[0-9]+}}:17 | function(Gen,TS)/C++ | foo | c:@F@foo<#{n64C>#*C# | __Z3fooIA64_cEvT_ | Decl,RelSpecialization | rel: 1
+template<> void foo(char[64]);
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -816,6 +816,25 @@
T = VT->getElementType();
continue;
}
+if (const auto *const AT = dyn_cast(T)) {
+ Out << '{';
+ switch (AT->getSizeModifier()) {
+ case ArrayType::Static:
+Out << 's';
+break;
+ case ArrayType::Star:
+Out << '*';
+break;
+ case ArrayType::Normal:
+Out << 'n';
+break;
+ }
+ if (const auto *const CAT = dyn_cast(T))
+Out << CAT->getSize();
+
+ T = AT->getElementType();
+ continue;
+}
// Unhandled type.
Out << ' ';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38707: PR13575: Fix USR mangling for functions taking function pointers as arguments.
jkorous-apple created this revision.
https://reviews.llvm.org/D38707
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/func-type.cpp
Index: test/Index/USR/func-type.cpp
===
--- /dev/null
+++ test/Index/USR/func-type.cpp
@@ -0,0 +1,13 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Functions taking function pointer parameters with different signatures
should result in unique USRs.
+
+typedef void (*_VoidToVoidPtr_)();
+typedef void (*_IntToVoidPtr_)( int );
+typedef _VoidToVoidPtr_ (*IntTo_VoidToVoidPtr_Ptr)( int );
+typedef _IntToVoidPtr_ (*VoidTo_IntToVoidPtr_Ptr)();
+
+void Func( IntTo_VoidToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv()(#I)# |
+void Func( VoidTo_IntToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)()# |
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -754,8 +754,12 @@
if (const FunctionProtoType *FT = T->getAs()) {
Out << 'F';
VisitType(FT->getReturnType());
- for (const auto &I : FT->param_types())
+ Out << '(';
+ for (const auto &I : FT->param_types()) {
+Out << '#';
VisitType(I);
+ }
+ Out << ')';
if (FT->isVariadic())
Out << '.';
return;
Index: test/Index/USR/func-type.cpp
===
--- /dev/null
+++ test/Index/USR/func-type.cpp
@@ -0,0 +1,13 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Functions taking function pointer parameters with different signatures should result in unique USRs.
+
+typedef void (*_VoidToVoidPtr_)();
+typedef void (*_IntToVoidPtr_)( int );
+typedef _VoidToVoidPtr_ (*IntTo_VoidToVoidPtr_Ptr)( int );
+typedef _IntToVoidPtr_ (*VoidTo_IntToVoidPtr_Ptr)();
+
+void Func( IntTo_VoidToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv()(#I)# |
+void Func( VoidTo_IntToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)()# |
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -754,8 +754,12 @@
if (const FunctionProtoType *FT = T->getAs()) {
Out << 'F';
VisitType(FT->getReturnType());
- for (const auto &I : FT->param_types())
+ Out << '(';
+ for (const auto &I : FT->param_types()) {
+Out << '#';
VisitType(I);
+ }
+ Out << ')';
if (FT->isVariadic())
Out << '.';
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38707: PR13575: Fix USR mangling for functions taking function pointers as arguments.
jkorous-apple updated this revision to Diff 118287.
jkorous-apple added a comment.
added another test
https://reviews.llvm.org/D38707
Files:
lib/Index/USRGeneration.cpp
test/Index/USR/func-type.cpp
Index: test/Index/USR/func-type.cpp
===
--- /dev/null
+++ test/Index/USR/func-type.cpp
@@ -0,0 +1,18 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Functions taking function pointer parameters with different signatures
should result in unique USRs.
+
+typedef void (*_VoidToVoidPtr_)();
+typedef void (*_IntToVoidPtr_)( int );
+typedef _VoidToVoidPtr_ (*IntTo_VoidToVoidPtr_Ptr)( int );
+typedef _IntToVoidPtr_ (*VoidTo_IntToVoidPtr_Ptr)();
+
+void Func( IntTo_VoidToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv()(#I)# |
+void Func( VoidTo_IntToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)()# |
+
+void Func( void (* (*)(int, int))(int, int) );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I#I)(#I#I)# |
+void Func( void (* (*)(int, int, int))(int) );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)(#I#I#I)# |
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -754,8 +754,12 @@
if (const FunctionProtoType *FT = T->getAs()) {
Out << 'F';
VisitType(FT->getReturnType());
- for (const auto &I : FT->param_types())
+ Out << '(';
+ for (const auto &I : FT->param_types()) {
+Out << '#';
VisitType(I);
+ }
+ Out << ')';
if (FT->isVariadic())
Out << '.';
return;
Index: test/Index/USR/func-type.cpp
===
--- /dev/null
+++ test/Index/USR/func-type.cpp
@@ -0,0 +1,18 @@
+// RUN: c-index-test core -print-source-symbols -- %s | FileCheck %s
+
+// Functions taking function pointer parameters with different signatures should result in unique USRs.
+
+typedef void (*_VoidToVoidPtr_)();
+typedef void (*_IntToVoidPtr_)( int );
+typedef _VoidToVoidPtr_ (*IntTo_VoidToVoidPtr_Ptr)( int );
+typedef _IntToVoidPtr_ (*VoidTo_IntToVoidPtr_Ptr)();
+
+void Func( IntTo_VoidToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv()(#I)# |
+void Func( VoidTo_IntToVoidPtr_Ptr );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)()# |
+
+void Func( void (* (*)(int, int))(int, int) );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I#I)(#I#I)# |
+void Func( void (* (*)(int, int, int))(int) );
+// CHECK: {{[0-9]+}}:6 | function/C | Func | c:@F@Func#*F*Fv(#I)(#I#I#I)# |
Index: lib/Index/USRGeneration.cpp
===
--- lib/Index/USRGeneration.cpp
+++ lib/Index/USRGeneration.cpp
@@ -754,8 +754,12 @@
if (const FunctionProtoType *FT = T->getAs()) {
Out << 'F';
VisitType(FT->getReturnType());
- for (const auto &I : FT->param_types())
+ Out << '(';
+ for (const auto &I : FT->param_types()) {
+Out << '#';
VisitType(I);
+ }
+ Out << ')';
if (FT->isVariadic())
Out << '.';
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38707: PR13575: Fix USR mangling for functions taking function pointers as arguments.
jkorous-apple added inline comments.
Comment at: lib/Index/USRGeneration.cpp:757
VisitType(FT->getReturnType());
- for (const auto &I : FT->param_types())
+ Out << '(';
+ for (const auto &I : FT->param_types()) {
arphaman wrote:
> I believe you can drop the '(' and ')'. The '#' should be enough to prevent
> the USR collision.
Actually, this is a great question!
I was worried it might not be the case so I created two more test cases.
Let's try to imagine there are no parentheses in those USRs.
First two test cases could be solved by using 'v' or something for functions
taking no arguments. But 3rd and 4th test cases would definitely need some
disambiguation then.
I am not saying this is the only possible way how to represent function
pointers but for proposed solution those parentheses are needed.
https://reviews.llvm.org/D38707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38711: typos in documentation?
jkorous-apple created this revision. I have found two possible typos in documentation. Since English is not my first language I would like to ask for verification. https://reviews.llvm.org/D38711 Files: docs/InternalsManual.rst Index: docs/InternalsManual.rst === --- docs/InternalsManual.rst +++ docs/InternalsManual.rst @@ -1540,7 +1540,7 @@ After trying several different approaches, we've finally converged on a design (Note, at the time of this writing, not all of this has been implemented, consider this a design goal!). Our basic approach is to define a single -recursive method evaluation method (``Expr::Evaluate``), which is implemented +recursive evaluation method (``Expr::Evaluate``), which is implemented in ``AST/ExprConstant.cpp``. Given an expression with "scalar" type (integer, fp, complex, or pointer) this method returns the following information: @@ -2037,7 +2037,7 @@ * ``CodeGenFunction`` contains functions ``ConvertType`` and ``ConvertTypeForMem`` that convert Clang's types (``clang::Type*`` or ``clang::QualType``) to LLVM types. Use the former for values, and the - later for memory locations: test with the C++ "``bool``" type to check + latter for memory locations: test with the C++ "``bool``" type to check this. If you find that you are having to use LLVM bitcasts to make the subexpressions of your expression have the type that your expression expects, STOP! Go fix semantic analysis and the AST so that you don't Index: docs/InternalsManual.rst === --- docs/InternalsManual.rst +++ docs/InternalsManual.rst @@ -1540,7 +1540,7 @@ After trying several different approaches, we've finally converged on a design (Note, at the time of this writing, not all of this has been implemented, consider this a design goal!). Our basic approach is to define a single -recursive method evaluation method (``Expr::Evaluate``), which is implemented +recursive evaluation method (``Expr::Evaluate``), which is implemented in ``AST/ExprConstant.cpp``. Given an expression with "scalar" type (integer, fp, complex, or pointer) this method returns the following information: @@ -2037,7 +2037,7 @@ * ``CodeGenFunction`` contains functions ``ConvertType`` and ``ConvertTypeForMem`` that convert Clang's types (``clang::Type*`` or ``clang::QualType``) to LLVM types. Use the former for values, and the - later for memory locations: test with the C++ "``bool``" type to check + latter for memory locations: test with the C++ "``bool``" type to check this. If you find that you are having to use LLVM bitcasts to make the subexpressions of your expression have the type that your expression expects, STOP! Go fix semantic analysis and the AST so that you don't ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38755: Fixed crash during indexing default template template param
jkorous-apple created this revision.
fix + testcase
https://reviews.llvm.org/D38755
Files:
lib/Index/IndexDecl.cpp
test/Index/index-template-template-param.cpp
Index: test/Index/index-template-template-param.cpp
===
--- /dev/null
+++ test/Index/index-template-template-param.cpp
@@ -0,0 +1,7 @@
+// RUN: c-index-test -index-file %s -x objective-c++ | FileCheck %s
+
+template class Template1 {};
+
+template class TMPL = Template1> class Template2;
+
+// CHECK: [indexEntityReference]: kind: c++-class-template | name: Template1 |
Index: lib/Index/IndexDecl.cpp
===
--- lib/Index/IndexDecl.cpp
+++ lib/Index/IndexDecl.cpp
@@ -666,7 +666,6 @@
}
bool VisitTemplateDecl(const TemplateDecl *D) {
-// FIXME: Template parameters.
// Index the default values for the template parameters.
const NamedDecl *Parent = D->getTemplatedDecl();
@@ -683,7 +682,7 @@
} else if (const auto *TTPD = dyn_cast(TP)) {
if (TTPD->hasDefaultArgument())
handleTemplateArgumentLoc(TTPD->getDefaultArgument(), Parent,
- /*DC=*/nullptr);
+ TP->getLexicalDeclContext());
}
}
}
Index: test/Index/index-template-template-param.cpp
===
--- /dev/null
+++ test/Index/index-template-template-param.cpp
@@ -0,0 +1,7 @@
+// RUN: c-index-test -index-file %s -x objective-c++ | FileCheck %s
+
+template class Template1 {};
+
+template class TMPL = Template1> class Template2;
+
+// CHECK: [indexEntityReference]: kind: c++-class-template | name: Template1 |
Index: lib/Index/IndexDecl.cpp
===
--- lib/Index/IndexDecl.cpp
+++ lib/Index/IndexDecl.cpp
@@ -666,7 +666,6 @@
}
bool VisitTemplateDecl(const TemplateDecl *D) {
-// FIXME: Template parameters.
// Index the default values for the template parameters.
const NamedDecl *Parent = D->getTemplatedDecl();
@@ -683,7 +682,7 @@
} else if (const auto *TTPD = dyn_cast(TP)) {
if (TTPD->hasDefaultArgument())
handleTemplateArgumentLoc(TTPD->getDefaultArgument(), Parent,
- /*DC=*/nullptr);
+ TP->getLexicalDeclContext());
}
}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38863: Typos in tutorial
jkorous-apple created this revision. https://reviews.llvm.org/D38863 Files: www/hacking.html Index: www/hacking.html === --- www/hacking.html +++ www/hacking.html @@ -246,9 +246,9 @@ For example: - python C:\Tool\llvm\utils\lit\lit.py -sv + python C:\Tools\llvm\utils\lit\lit.py -sv --param=build_mode=Win32 --param=build_config=Debug - --param=clang_site_config=c:\Tools\build\tools\clang\test\lit.site.cfg + --param=clang_site_config=C:\Tools\build\tools\clang\test\lit.site.cfg C:\Tools\llvm\tools\clang\test\Sema\wchar.c Index: www/hacking.html === --- www/hacking.html +++ www/hacking.html @@ -246,9 +246,9 @@ For example: - python C:\Tool\llvm\utils\lit\lit.py -sv + python C:\Tools\llvm\utils\lit\lit.py -sv --param=build_mode=Win32 --param=build_config=Debug - --param=clang_site_config=c:\Tools\build\tools\clang\test\lit.site.cfg + --param=clang_site_config=C:\Tools\build\tools\clang\test\lit.site.cfg C:\Tools\llvm\tools\clang\test\Sema\wchar.c ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr
jkorous created this revision.
jkorous added reviewers: rsmith, doug.gregor, JDevlieghere, thegameg.
jkorous added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
Use logical or operator instead of a bool variable and if/else.
Repository:
rC Clang
https://reviews.llvm.org/D51485
Files:
lib/Sema/SemaExpr.cpp
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5330,13 +5330,7 @@
// Determine whether this is a dependent call inside a C++ template,
// in which case we won't do any semantic analysis now.
-bool Dependent = false;
-if (Fn->isTypeDependent())
- Dependent = true;
-else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
- Dependent = true;
-
-if (Dependent) {
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))
{
if (ExecConfig) {
return new (Context) CUDAKernelCallExpr(
Context, Fn, cast(ExecConfig), ArgExprs,
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5330,13 +5330,7 @@
// Determine whether this is a dependent call inside a C++ template,
// in which case we won't do any semantic analysis now.
-bool Dependent = false;
-if (Fn->isTypeDependent())
- Dependent = true;
-else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
- Dependent = true;
-
-if (Dependent) {
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs)) {
if (ExecConfig) {
return new (Context) CUDAKernelCallExpr(
Context, Fn, cast(ExecConfig), ArgExprs,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51485: [Sema][NFC] Trivial cleanup in ActOnCallExpr
This revision was automatically updated to reflect the committed changes.
Closed by commit rL341074: [Sema][NFC] Trivial cleanup in ActOnCallExpr
(authored by jkorous, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D51485?vs=163304&id=163327#toc
Repository:
rL LLVM
https://reviews.llvm.org/D51485
Files:
cfe/trunk/lib/Sema/SemaExpr.cpp
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -5330,13 +5330,7 @@
// Determine whether this is a dependent call inside a C++ template,
// in which case we won't do any semantic analysis now.
-bool Dependent = false;
-if (Fn->isTypeDependent())
- Dependent = true;
-else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
- Dependent = true;
-
-if (Dependent) {
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))
{
if (ExecConfig) {
return new (Context) CUDAKernelCallExpr(
Context, Fn, cast(ExecConfig), ArgExprs,
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -5330,13 +5330,7 @@
// Determine whether this is a dependent call inside a C++ template,
// in which case we won't do any semantic analysis now.
-bool Dependent = false;
-if (Fn->isTypeDependent())
- Dependent = true;
-else if (Expr::hasAnyTypeDependentArguments(ArgExprs))
- Dependent = true;
-
-if (Dependent) {
+if (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs)) {
if (ExecConfig) {
return new (Context) CUDAKernelCallExpr(
Context, Fn, cast(ExecConfig), ArgExprs,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?
jkorous created this revision.
jkorous added reviewers: rsmith, vsapsai, JDevlieghere.
jkorous added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
Since there's no change to ArgExprs between here and the previous early exit
starting at L 5333 it's effectively a dead code.
On the other hand a possible counter-argument is that code gets a bit more
brittle.
WDYT?
Repository:
rC Clang
https://reviews.llvm.org/D51488
Files:
Sema/SemaExpr.cpp
Index: Sema/SemaExpr.cpp
===
--- Sema/SemaExpr.cpp
+++ Sema/SemaExpr.cpp
@@ -5369,9 +5369,6 @@
// We aren't supposed to apply this logic if there's an '&' involved.
if (!find.HasFormOfMemberPointer) {
- if (Expr::hasAnyTypeDependentArguments(ArgExprs))
-return new (Context) CallExpr(
-Context, Fn, ArgExprs, Context.DependentTy, VK_RValue, RParenLoc);
OverloadExpr *ovl = find.Expression;
if (UnresolvedLookupExpr *ULE = dyn_cast(ovl))
return BuildOverloadedCallExpr(
Index: Sema/SemaExpr.cpp
===
--- Sema/SemaExpr.cpp
+++ Sema/SemaExpr.cpp
@@ -5369,9 +5369,6 @@
// We aren't supposed to apply this logic if there's an '&' involved.
if (!find.HasFormOfMemberPointer) {
- if (Expr::hasAnyTypeDependentArguments(ArgExprs))
-return new (Context) CallExpr(
-Context, Fn, ArgExprs, Context.DependentTy, VK_RValue, RParenLoc);
OverloadExpr *ovl = find.Expression;
if (UnresolvedLookupExpr *ULE = dyn_cast(ovl))
return BuildOverloadedCallExpr(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51543: [Sema] Fix uninitialized OverloadCandidate::FoundDecl member
jkorous created this revision.
jkorous added reviewers: arphaman, vsapsai, dexonsmith, rsmith.
jkorous added a project: clang.
Herald added subscribers: cfe-commits, mgorny.
It's rather error-prone to leave that member variable uninitialized. The
DeclAccessPair seems to be intentionally POD and it seems the make() method is
supposed to be called as a constructor.
I take DeclAccessPair interface as given so the only options are to either
change OverloadCandidate interface or actually initialize the member variable
it to some sane default. I suggest the easiest option.
Repository:
rC Clang
https://reviews.llvm.org/D51543
Files:
Sema/CMakeLists.txt
Sema/OverloadTest.cpp
clang/Sema/Overload.h
Index: Sema/OverloadTest.cpp
===
--- /dev/null
+++ Sema/OverloadTest.cpp
@@ -0,0 +1,21 @@
+//=== unittests/Sema/CodeCompleteTest.cpp - Code Complete tests
==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Sema/Overload.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+TEST(SemaOverloadTest, FoundDeclIsInitializedInOverloadCandidate) {
+ clang::OverloadCandidate foo;
+ EXPECT_EQ(foo.FoundDecl.getDecl(), nullptr);
+ EXPECT_EQ(foo.FoundDecl.getAccess(), clang::AS_none);
+}
+
+} // namespace
Index: Sema/CMakeLists.txt
===
--- Sema/CMakeLists.txt
+++ Sema/CMakeLists.txt
@@ -5,6 +5,7 @@
add_clang_unittest(SemaTests
ExternalSemaSourceTest.cpp
CodeCompleteTest.cpp
+ OverloadTest.cpp
)
target_link_libraries(SemaTests
Index: clang/Sema/Overload.h
===
--- clang/Sema/Overload.h
+++ clang/Sema/Overload.h
@@ -728,6 +728,11 @@
/// OverloadCandidate - A single candidate in an overload set (C++ 13.3).
struct OverloadCandidate {
+
+OverloadCandidate()
+: FoundDecl( DeclAccessPair::make(nullptr, AS_none))
+{ }
+
/// Function - The actual function that this candidate
/// represents. When NULL, this is a built-in candidate
/// (C++ [over.oper]) or a surrogate for a conversion to a
Index: Sema/OverloadTest.cpp
===
--- /dev/null
+++ Sema/OverloadTest.cpp
@@ -0,0 +1,21 @@
+//=== unittests/Sema/CodeCompleteTest.cpp - Code Complete tests ==//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Sema/Overload.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+TEST(SemaOverloadTest, FoundDeclIsInitializedInOverloadCandidate) {
+ clang::OverloadCandidate foo;
+ EXPECT_EQ(foo.FoundDecl.getDecl(), nullptr);
+ EXPECT_EQ(foo.FoundDecl.getAccess(), clang::AS_none);
+}
+
+} // namespace
Index: Sema/CMakeLists.txt
===
--- Sema/CMakeLists.txt
+++ Sema/CMakeLists.txt
@@ -5,6 +5,7 @@
add_clang_unittest(SemaTests
ExternalSemaSourceTest.cpp
CodeCompleteTest.cpp
+ OverloadTest.cpp
)
target_link_libraries(SemaTests
Index: clang/Sema/Overload.h
===
--- clang/Sema/Overload.h
+++ clang/Sema/Overload.h
@@ -728,6 +728,11 @@
/// OverloadCandidate - A single candidate in an overload set (C++ 13.3).
struct OverloadCandidate {
+
+OverloadCandidate()
+: FoundDecl( DeclAccessPair::make(nullptr, AS_none))
+{ }
+
/// Function - The actual function that this candidate
/// represents. When NULL, this is a built-in candidate
/// (C++ [over.oper]) or a surrogate for a conversion to a
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?
jkorous added a comment.
Sorry for the delay - I was busy with other things for past two weeks.
Comment at: Sema/SemaExpr.cpp:5338
Context.DependentTy, VK_RValue, RParenLoc);
} else {
JDevlieghere wrote:
> While you're at it you might as well remove the else branch here.
Sorry, I am not following. Do you mean just refactoring?
```
if a
foo
else
bar
```
->
```
if a
foo
bar
```
Or do you really mean removing the else branch? I don't see how that would be
NFC.
Repository:
rC Clang
https://reviews.llvm.org/D51488
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51867: [Diagnostics][NFC] Add error handling to FormatDiagnostic()
jkorous created this revision.
jkorous added reviewers: arphaman, vsapsai.
jkorous added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
There seem to be couple implicit assumptions that might be better represented
explicitly by asserts.
Repository:
rC Clang
https://reviews.llvm.org/D51867
Files:
lib/Basic/Diagnostic.cpp
Index: lib/Basic/Diagnostic.cpp
===
--- lib/Basic/Diagnostic.cpp
+++ lib/Basic/Diagnostic.cpp
@@ -768,6 +768,7 @@
void Diagnostic::
FormatDiagnostic(const char *DiagStr, const char *DiagEnd,
SmallVectorImpl &OutStr) const {
+ assert(DiagStr <= DiagEnd && "Invalid DiagStr-DiagEnd range");
// When the diagnostic string is only "%0", the entire string is being given
// by an outside source. Remove unprintable characters from this string
// and skip all the other string processing.
@@ -798,17 +799,22 @@
if (getArgKind(i) == DiagnosticsEngine::ak_qualtype)
QualTypeVals.push_back(getRawArg(i));
- while (DiagStr != DiagEnd) {
+ assert(DiagStr != nullptr && "DiagStr is nullptr");
+
+ while (DiagStr < DiagEnd) {
if (DiagStr[0] != '%') {
// Append non-%0 substrings to Str if we have one.
const char *StrEnd = std::find(DiagStr, DiagEnd, '%');
OutStr.append(DiagStr, StrEnd);
DiagStr = StrEnd;
continue;
-} else if (isPunctuation(DiagStr[1])) {
+} else if ((DiagStr + 1) < DiagEnd && isPunctuation(DiagStr[1])) {
OutStr.push_back(DiagStr[1]); // %% -> %.
DiagStr += 2;
continue;
+} else if ((DiagStr + 1) >= DiagEnd) {
+ llvm_unreachable("DiagStr ends with '%'");
+ return;
}
// Skip the %.
@@ -825,34 +831,40 @@
// Check to see if we have a modifier. If so eat it.
if (!isDigit(DiagStr[0])) {
Modifier = DiagStr;
- while (DiagStr[0] == '-' ||
- (DiagStr[0] >= 'a' && DiagStr[0] <= 'z'))
+ while (DiagStr < DiagEnd &&
+ (DiagStr[0] == '-' || (DiagStr[0] >= 'a' && DiagStr[0] <= 'z')))
++DiagStr;
ModifierLen = DiagStr-Modifier;
// If we have an argument, get it next.
if (DiagStr[0] == '{') {
++DiagStr; // Skip {.
+assert(DiagStr < DiagEnd && "Invalid DiagStr");
Argument = DiagStr;
DiagStr = ScanFormat(DiagStr, DiagEnd, '}');
assert(DiagStr != DiagEnd && "Mismatched {}'s in diagnostic string!");
ArgumentLen = DiagStr-Argument;
++DiagStr; // Skip }.
+assert(DiagStr < DiagEnd && "Invalid DiagStr");
}
}
assert(isDigit(*DiagStr) && "Invalid format for argument in diagnostic");
-unsigned ArgNo = *DiagStr++ - '0';
+
+unsigned ArgNo = *DiagStr - '0';
// Only used for type diffing.
unsigned ArgNo2 = ArgNo;
+++DiagStr;
DiagnosticsEngine::ArgumentKind Kind = getArgKind(ArgNo);
if (ModifierIs(Modifier, ModifierLen, "diff")) {
+ assert(DiagStr + 1 < DiagEnd && "Invalid diff modifier in DiagStr");
assert(*DiagStr == ',' && isDigit(*(DiagStr + 1)) &&
"Invalid format for diff modifier");
++DiagStr; // Comma.
+ assert(DiagStr < DiagEnd && "Invalid DiagStr");
ArgNo2 = *DiagStr++ - '0';
DiagnosticsEngine::ArgumentKind Kind2 = getArgKind(ArgNo2);
if (Kind == DiagnosticsEngine::ak_qualtype &&
Index: lib/Basic/Diagnostic.cpp
===
--- lib/Basic/Diagnostic.cpp
+++ lib/Basic/Diagnostic.cpp
@@ -768,6 +768,7 @@
void Diagnostic::
FormatDiagnostic(const char *DiagStr, const char *DiagEnd,
SmallVectorImpl &OutStr) const {
+ assert(DiagStr <= DiagEnd && "Invalid DiagStr-DiagEnd range");
// When the diagnostic string is only "%0", the entire string is being given
// by an outside source. Remove unprintable characters from this string
// and skip all the other string processing.
@@ -798,17 +799,22 @@
if (getArgKind(i) == DiagnosticsEngine::ak_qualtype)
QualTypeVals.push_back(getRawArg(i));
- while (DiagStr != DiagEnd) {
+ assert(DiagStr != nullptr && "DiagStr is nullptr");
+
+ while (DiagStr < DiagEnd) {
if (DiagStr[0] != '%') {
// Append non-%0 substrings to Str if we have one.
const char *StrEnd = std::find(DiagStr, DiagEnd, '%');
OutStr.append(DiagStr, StrEnd);
DiagStr = StrEnd;
continue;
-} else if (isPunctuation(DiagStr[1])) {
+} else if ((DiagStr + 1) < DiagEnd && isPunctuation(DiagStr[1])) {
OutStr.push_back(DiagStr[1]); // %% -> %.
DiagStr += 2;
continue;
+} else if ((DiagStr + 1) >= DiagEnd) {
+ llvm_unreachable("DiagStr ends with '%'");
+ return;
}
// Skip the %.
@@ -825,34 +831,40 @@
// Check to see if we have a modifier. If so eat it.
if (!isDigit(DiagStr[0])) {
Modifier = DiagStr;
- while (DiagStr[0] ==
[PATCH] D51867: [Diagnostics] Add error handling to FormatDiagnostic()
jkorous planned changes to this revision.
jkorous added a comment.
Hi Volodymyr,
Thanks for the feedback - interesting points!
I see your point regarding NFC - I am going to drop it as you are right.
Two things about sanitizers come to mind:
1. You'd need to guarantee that you have all possible code paths (or at least
those you are able to cover with asserts) covered in tests to be able to
replace asserts with sanitizers. (I think that even if that would be feasible
asserts would prove to be way simpler.)
2. I prefer explicit assert right in the place where an assumption is made to
test somewhere else as it make understanding the code much easier.
> Those change loop iteration from "iterator != end" to "iterator < end". As it
> is functionality change, I'd like to have tests to cover that.
Technically you are right but I assume (ok, busted) that without any bug in the
iteration this is NFC. I will try to look if I can find some simple input that
would break current implementation.
> Also I've fixed a few bugs with going past the end of buffer and bugs were
> actually inside the loop, not with buffer range check. It is tempting to play
> safe but it has a risk of hiding real bugs.
But that almost sounds as that we should write fragile code so bugs from other
parts of codebase show up... Anyway, I will think about this a bit more, it's
an interesting point.
Comment at: lib/Basic/Diagnostic.cpp:804
+
+ while (DiagStr < DiagEnd) {
if (DiagStr[0] != '%') {
vsapsai wrote:
> For example, I wouldn't call this one NFC.
You are right, I am gonna drop the NFC.
Repository:
rC Clang
https://reviews.llvm.org/D51867
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51867: [Diagnostics] Add error handling to FormatDiagnostic()
jkorous added a comment. I tried to come up with some input that breaks current implementation so I could add the test. Problem is that invalid memory read doesn't guarantee deterministic crash. E. g. with this input the current implementation is definitely reading way past the buffer: SmallVector IgnoreMe; const char* Foo = "foo%"; const char* FooEnd = Foo + 4; Diag.FormatDiagnostic(Foo, FooEnd, IgnoreMe); ...and it actually found some string there yet it didn't crash until it hit some unrelated assert (lldb) p DiagStr (const char *) $0 = 0x000100adc53b " SplatSizeInBits == 0 && \"SplatSizeInBits must divide width!\"" (lldb) p *DiagStr (const char) $1 = ' ' (lldb) p DiagEnd (const char *) $2 = 0x000100ad4155 "0" The only reliable fail is passing nullptr which currently leads to SIGABRT (nullptr dereferenced) SmallVector IgnoreMe; const char* Foo = "foo"; Diag.FormatDiagnostic(Foo, nullptr, IgnoreMe); I am reconsidering the necessity of such tests here. WDYT? Repository: rC Clang https://reviews.llvm.org/D51867 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51488: [Sema][NFC] Small cleanup - remove dead code from ActOnCallExpr() ?
jkorous added a comment. I got some test failure with the patch, still investigating the issue. Repository: rC Clang https://reviews.llvm.org/D51488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48560: [clangd] JSON <-> XPC conversions
jkorous marked 12 inline comments as done.
jkorous added inline comments.
Comment at: xpc/XPCJSONConversions.cpp:62
+ if (objType == XPC_TYPE_UINT64)
+return json::Expr(xpc_uint64_get_value(xpcObj));
+ if (objType == XPC_TYPE_STRING)
sammccall wrote:
> hmm, I think this shouldn't compile anymore, rather require you to explicitly
> do the narrowing conversion to int64 or double.
Explicitly narroving now. Thanks.
Comment at: xpc/XPCJSONConversions.cpp:69
+ xpcObj,
+ ^bool(size_t /* index */, xpc_object_t value) {
+result.emplace_back(xpcToJson(value));
jkorous wrote:
> sammccall wrote:
> > this looks like objC syntax, I'm not familiar with it, but should this file
> > be `.mm`?
> >
> > Alternatively, it seems like you could write a C++ loop with
> > `xpc_array_get_value` (though I don't know if there are performance
> > concerns).
> >
> > (and dictionary)
> Oh, sorry. These are actually C blocks - a nonstandard C extension.
> https://en.wikipedia.org/wiki/Blocks_(C_language_extension)
> https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Blocks/Articles/00_Introduction.html
>
> There was no performance concern on my side - just liked this approach
> better. Didn't realize how confusing it might be, will rewrite this.
Allright, by trying to get rid of C block in dictionary conversion I remembered
that there's unfortunately no sane reason how to iterate XPC dictionary without
using xpc_dictionary_apply() which uses C block for functor parameter.
https://developer.apple.com/documentation/xpc/1505404-xpc_dictionary_apply?language=objc
https://developer.apple.com/documentation/xpc/xpc_dictionary_applier_t?language=objc
Anyway, even had we succeeded in removing C blocks from conversion they still
would be necessary for dispatch as xpc_connection_set_event_handler() is
another part of XPC API that uses it.
I guess that there's no point in removing the xpc_array_apply() then. Is that
ok with you?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48560
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48562: [clangd] XPC transport layer
jkorous marked 7 inline comments as done. jkorous added inline comments. Comment at: xpc/test-client/ClangdXPCTestClient.cpp:51 + dlHandle, "clangd_xpc_get_bundle_identifier"); + xpc_connection_t conn = + xpc_connection_create(clangd_xpc_get_bundle_identifier(), NULL); arphaman wrote: > jkorous wrote: > > arphaman wrote: > > > We should probably use the main queue here too to ensure that we don't > > > get bit by concurrent handler invocations. > > As far as I understand it this is taken care of by calling dispatch_main(). > > > > Per man dispatch_main: > > "MAIN QUEUE > > [...] > > Programs must call dispatch_main() at the end of main() in order to > > process blocks submitted to the main queue." > > > Not really. The documentation for the parameter states the following: > > The GCD queue to which the event handler block will be submitted. This > parameter may be NULL, in which case the connection's target queue will be > libdispatch's default target queue, defined as DISPATCH_TARGET_QUEUE_DEFAULT. > The target queue may be changed later with a call to > xpc_connection_set_target_queue(). > > So the handler blocks will be submitted to the concurrent queue > (DISPATCH_TARGET_QUEUE_DEFAULT) which means that they might end up running > concurrently. Sorry, my bad. https://reviews.llvm.org/D48562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49548: [clangd] XPC WIP
jkorous created this revision.
jkorous added reviewers: sammccall, arphaman.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, ioeric,
ilya-biryukov, mgorny.
Combined, rebased and modified original XPC patches.
- https://reviews.llvm.org/D48559 [clangd] refactoring for XPC transport layer
[NFCI]
- https://reviews.llvm.org/D48560 [clangd] JSON <-> XPC conversions
- https://reviews.llvm.org/D48562 [clangd] XPC transport layer
- Rebased on TOT.
- Made changes according to review.
Couple of questions are still open in reviews of original patches and more
importantly the discussion about transport abstraction just started:
https://reviews.llvm.org/D48559#1167340
https://reviews.llvm.org/D49389
I thought it would be useful to have current state of our original approach
online as it will be foundation for our implementation anyway (implementing
whatever interface we converge to).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49548
Files:
CMakeLists.txt
ClangdLSPServer.cpp
ClangdLSPServer.h
DispatcherCommon.cpp
DispatcherCommon.h
Features.inc.in
JSONRPCDispatcher.cpp
JSONRPCDispatcher.h
LSPOutput.h
ProtocolHandlers.cpp
ProtocolHandlers.h
clangd/CMakeLists.txt
clangd/xpc/CMakeLists.txt
clangd/xpc/ConversionTests.cpp
clangd/xpc/initialize.test
lit.cfg
lit.site.cfg.in
tool/CMakeLists.txt
tool/ClangdMain.cpp
xpc/CMakeLists.txt
xpc/Conversion.cpp
xpc/Conversion.h
xpc/README.txt
xpc/XPCDispatcher.cpp
xpc/XPCDispatcher.h
xpc/cmake/Info.plist.in
xpc/cmake/XPCServiceInfo.plist.in
xpc/cmake/modules/CreateClangdXPCFramework.cmake
xpc/framework/CMakeLists.txt
xpc/framework/ClangdXPC.cpp
xpc/test-client/CMakeLists.txt
xpc/test-client/ClangdXPCTestClient.cpp
Index: clangd/xpc/ConversionTests.cpp
===
--- /dev/null
+++ clangd/xpc/ConversionTests.cpp
@@ -0,0 +1,454 @@
+//===-- ConversionTests.cpp --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/Conversion.h"
+#include "gtest/gtest.h"
+
+#include
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+TEST(JsonXpcConversionTest, Null) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(nullptr)),
+ xpc_null_create()
+)
+ );
+ EXPECT_TRUE(
+json::Value(nullptr) == xpcToJson(xpc_null_create())
+ );
+}
+
+TEST(JsonXpcConversionTest, Bool) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(true)),
+ xpc_bool_create(true)
+)
+ );
+ EXPECT_TRUE(
+json::Value(false) == xpcToJson(xpc_bool_create(false))
+ );
+}
+
+TEST(JsonXpcConversionTest, Number) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(3.14)),
+ xpc_double_create(3.14)
+)
+ );
+ EXPECT_TRUE(
+json::Value(3.14) == xpcToJson(xpc_double_create(3.14))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(42)),
+ xpc_double_create(42)
+)
+ );
+ EXPECT_TRUE(
+json::Value(42) == xpcToJson(xpc_double_create(42))
+ );
+ EXPECT_TRUE(
+json::Value(42) == xpcToJson(xpc_int64_create(42))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(-100)),
+ xpc_double_create(-100)
+)
+ );
+ EXPECT_TRUE(
+json::Value(-100) == xpcToJson(xpc_double_create(-100))
+ );
+
+ int64_t bigPositiveValue = std::numeric_limits::max();
+ ++bigPositiveValue;
+ int64_t bigNegativeValue = std::numeric_limits::min();
+ --bigNegativeValue;
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(bigPositiveValue)),
+ xpc_double_create(bigPositiveValue)
+)
+ );
+ EXPECT_TRUE(
+json::Value(bigPositiveValue) == xpcToJson(xpc_double_create(bigPositiveValue))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(bigNegativeValue)),
+ xpc_double_create(bigNegativeValue)
+)
+ );
+ EXPECT_TRUE(
+json::Value(bigNegativeValue) == xpcToJson(xpc_double_create(bigNegativeValue))
+ );
+}
+
+TEST(JsonXpcConversionTest, String) {
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value("foo")),
+ xpc_string_create("foo")
+)
+ );
+ EXPECT_TRUE(
+json::Value("foo") == xpcToJson(xpc_string_create("foo"))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value("")),
+ xpc_string_create("")
+)
+ );
+ EXPECT_TRUE(
+json::Value("") == xpcToJson(xpc_string_create(""))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value("123")),
+ xpc_string_create("123")
+)
+ );
+ EXPECT_TRUE(
+json::Value("123") == xpcToJson(xpc_string_create("123"))
+ );
+
+ EXPECT_TRUE(
+xpc_equal(
+ jsonToXpc(json::Value(" ")),
+ xpc_string_create
[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]
jkorous added a comment. Sam, just out of curiosity - would it be possible for you to share any relevant experience gained by using porting clangd to protobuf-based transport layer? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49523: [clangd] Add support for per-file override compilation command
jkorous added a comment. Hi Marc-Andre, what is a structure of data you are passing as parameter of didChangeConfiguration message? All we need is to pass per-file compilation command to clangd. Maybe we could send didChangeConfiguration message right after didOpen. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49523: [clangd] Add support for per-file override compilation command
jkorous added a comment. Alex, I am just wondering if we shouldn't rather create another implementation of GlobalCompilationDatabase interface (something like InMemoryGlobalCompilationDatabase), add it to ClangdServer and use it as the first place to be searched in ClangdServer::getCompileCommand(PathRef File). What do you think? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]
jkorous added a comment. Hi Sam, we discussed a bit more with Alex offline. Ultimately we don't mind using JSON as part of the generic interface but we'd like to optimize specific cases in the future (thinking only about code completion so far) which might need to "work-around" the abstraction. What is your opinion about this - does it ruin the whole point of having the abstraction or do you consider pragmatic to have abstraction covering most of the functionality with maybe couple exceptions? I kind of assume (please correct me if I am wrong) that you might have done something similar with profobuf interface so we're definitely happy to discuss. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48560: [clangd] JSON <-> XPC conversions
jkorous added a comment. BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48560 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48562: [clangd] XPC transport layer
jkorous added a comment. BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548 https://reviews.llvm.org/D48562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]
jkorous added a comment. BTW Just for the record - I put the rebased & updated patch here: [clangd] XPC WIP https://reviews.llvm.org/D49548 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49523: [clangd] Add support for per-file override compilation command
jkorous added a comment. BTW it looks like we already had kind of support for compilation command before (extra flags). commit 5ec1f7ca32eb85077a22ce81d41aa02a017d4852 Author: Krasimir Georgiev Date: Thu Jul 6 08:44:54 2017 + [clangd] Add support for per-file extra flags There is even an LSP extension proposal: https://github.com/Microsoft/language-server-protocol/issues/255 But it seems as if we lost access to it here: commit aa49372548ff984ae9c48879a0d05833538a76b3 Author: Sam McCall Date: Mon Dec 4 10:08:45 2017 + [clangd] GlobalCompilationDatabase interface changes Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49523 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's *similar* existing flag for given unknown one
jkorous created this revision.
jkorous added reviewers: arphaman, vsapsai, bruno, JDevlieghere.
Herald added subscribers: cfe-commits, dexonsmith.
In case user specified non-existent warning flag clang was suggesting an
existing on that was in some cased very different. This patch should ensure
that either a relevant warning flag is suggested or none (which is in line with
unknown generic command line option handling).
Examples:
warning: unknown warning option '-Whiskey'; did you mean '-Wpacked'?
warning: unknown warning option '-Weverthin'; did you mean '-Wsection'?
rdar://problem/37755482
I changed interface of DiagnosticIDs::getNearestOption() to return the edit
distance between nonexistent flag and the closest exiting one. In principle
this code is analogous to OptTable::findNearest() for generic command line
options but I don't find them similar enough in order to factor out some shared
implementation. I tentatively selected maximum edit distance as 2
(CompilerInvocation::CreateFromArgs() uses maximum edit distance of 1 for
generic case).
Repository:
rC Clang
https://reviews.llvm.org/D49736
Files:
Basic/DiagnosticIDs.cpp
Basic/Warnings.cpp
Driver/cc-log-diagnostics.c
Frontend/warning-options.cpp
clang/Basic/DiagnosticIDs.h
Index: Frontend/warning-options.cpp
===
--- Frontend/warning-options.cpp
+++ Frontend/warning-options.cpp
@@ -3,5 +3,5 @@
// CHECK: unknown warning option '-Wmonkey'
// CHECK: unknown warning option '-Wno-monkey'
// CHECK: unknown warning option '-Wno-unused-command-line-arguments'; did you mean '-Wno-unused-command-line-argument'?
-// CHECK: unknown warning option '-Wmodule-build'; did you mean '-Wmodule-conflict'?
+// CHECK: unknown warning option '-Wmodule-build'
// CHECK: unknown remark option '-Rmodule-built'; did you mean '-Rmodule-build'?
Index: Driver/cc-log-diagnostics.c
===
--- Driver/cc-log-diagnostics.c
+++ Driver/cc-log-diagnostics.c
@@ -17,7 +17,7 @@
// CHECK: level
// CHECK: warning
// CHECK: message
-// CHECK: unknown warning option '-Wfoobar'; did you mean '-W{{.*}}'?
+// CHECK: unknown warning option '-Wfoobar'
// CHECK:
// CHECK:
// CHECK: level
Index: Basic/Warnings.cpp
===
--- Basic/Warnings.cpp
+++ Basic/Warnings.cpp
@@ -30,15 +30,24 @@
#include
using namespace clang;
+namespace {
+ constexpr unsigned MaxLevenshteinDistForWarningSuggestion = 2;
+}
+
// EmitUnknownDiagWarning - Emit a warning and typo hint for unknown warning
// opts
static void EmitUnknownDiagWarning(DiagnosticsEngine &Diags,
diag::Flavor Flavor, StringRef Prefix,
StringRef Opt) {
- StringRef Suggestion = DiagnosticIDs::getNearestOption(Flavor, Opt);
+ std::string Suggestion;
+ const unsigned OptToSuggestionEditDist
+= DiagnosticIDs::getNearestOption(Flavor, Opt, Suggestion);
Diags.Report(diag::warn_unknown_diag_option)
<< (Flavor == diag::Flavor::WarningOrError ? 0 : 1) << (Prefix.str() += Opt)
-<< !Suggestion.empty() << (Prefix.str() += Suggestion);
+<< (!Suggestion.empty()
+ && OptToSuggestionEditDist > 0
+ && OptToSuggestionEditDist <= MaxLevenshteinDistForWarningSuggestion)
+<< (Prefix.str() += Suggestion);
}
void clang::ProcessWarningOptions(DiagnosticsEngine &Diags,
Index: Basic/DiagnosticIDs.cpp
===
--- Basic/DiagnosticIDs.cpp
+++ Basic/DiagnosticIDs.cpp
@@ -589,9 +589,9 @@
Diags.push_back(StaticDiagInfo[i].DiagID);
}
-StringRef DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
- StringRef Group) {
- StringRef Best;
+unsigned DiagnosticIDs::getNearestOption(diag::Flavor Flavor,
+ StringRef Group,
+ std::string& NearestOption) {
unsigned BestDistance = Group.size() + 1; // Sanity threshold.
for (const WarningOption &O : OptionTable) {
// Don't suggest ignored warning flags.
@@ -609,15 +609,15 @@
if (Distance == BestDistance) {
// Two matches with the same distance, don't prefer one over the other.
- Best = "";
+ NearestOption = "";
} else if (Distance < BestDistance) {
// This is a better match.
- Best = O.getName();
+ NearestOption = O.getName();
BestDistance = Distance;
}
}
- return Best;
+ return BestDistance;
}
/// ProcessDiag - This is the method used to report a diagnostic that is
Index: clang/Basic/DiagnosticIDs.h
===
--- clang/Basic/DiagnosticIDs.h
+++ clang/Basic/DiagnosticIDs.h
@@ -300,8 +300,12 @@
std::v
[PATCH] D48559: [clangd] refactoring for XPC transport layer [NFCI]
jkorous planned changes to this revision. jkorous added a comment. Hi Sam, we are still discussing internally how to fit clangd and XPC together. Please bear with us and ignore our patches until we decide. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48559 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one
jkorous added a comment. I like that idea! It looks like it's living in a wrong place anyway as it doesn't need access to any of implementation details (private members) of DiagnosticID. I would still like to preserve it as a function so this block of code has clear semantics and interface. How about I refactor it to a static free function in Warnings.cpp? Repository: rC Clang https://reviews.llvm.org/D49736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43230: [clangd] Explicitly initialize all primitive fields in Protocol.h
jkorous-apple added inline comments.
Comment at: clangd/Protocol.h:190
/// (see exit notification) its process.
- llvm::Optional processId;
+ llvm::Optional processId = 0;
Sorry if I am asking stupid question but since the type is Optional<> isn't
it's default-constructed value more appropriate here?
Comment at: clangd/Protocol.h:211
/// The initial trace setting. If omitted trace is disabled ('off').
- llvm::Optional trace;
+ llvm::Optional trace = TraceLevel::Off;
};
Does it still make more sense to use Optional than plain TraceLevel?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43331: [WIP][Parser][FixIt] Better diagnostics for omitted template keyword (type dependent template names)
jkorous-apple created this revision.
What I would love to have is FixIt for this kind of situation:
template void foo() {
T::bar(); /* missing template keyword */
}
which currently gets not that helpful diagnostics:
> clang misleading.cpp
misleading.cpp:2:15: error: expected '(' for function-style cast or type
construction
T::bar();
~~~^
misleading.cpp:2:16: error: expected expression
T::bar();
^
misleading.cpp:2:18: error: expected expression
T::bar();
^
3 errors generated.
while
template void foo() {
T::bar();
}
gets a good one:
good.cpp:2:8: error: use 'template' keyword to treat 'bar' as a dependent
template name
T::bar();
^
template
1 error generated.
If I understand it correctly than current implementation of
bool Parser::IsTemplateArgumentList(unsigned Skip)
relies on the fact that if there's a '<' token followed by declaration
specifier it has to be a template. Which is true but produces false negatives
for non-type parameters (array<1>) or pointer type parameters (vector)
(among others).
One particular thing to keep on mind is that there's the [over.over] rule about
resolving names to function pointers - best explained by test
test/SemaTemplate/dependent-template-recover.cpp (look for snippet below):
// Note: no diagnostic here, this is actually valid as a comparison between
// the decayed pointer to Y::g<> and mb!
T::g(0);
What I did so far is that in case the simple approach doesn't validate there's
a template I try to use speculative parsing. The only problem remaining is
diagnostics - I am able to use TentativeParsingAction which is later reverted
but I still get all the diagnostics which is incorrect in case my speculation
about how to parse the code was wrong.
https://reviews.llvm.org/D43331
Files:
FixIt/fixit-template-for-dependent-name.cpp
Parse/ParseTemplate.cpp
SemaTemplate/dependent-template-recover.cpp
Index: SemaTemplate/dependent-template-recover.cpp
===
--- SemaTemplate/dependent-template-recover.cpp
+++ SemaTemplate/dependent-template-recover.cpp
@@ -6,6 +6,7 @@
t->f0(); // expected-error{{use 'template' keyword to treat 'f0' as a
dependent template name}}
t->operator+(); // expected-error{{use 'template' keyword to
treat 'operator +' as a dependent template name}}
+t->operator+<1, U const>(); // expected-error{{use 'template' keyword to
treat 'operator +' as a dependent template name}}
t->f1(); // expected-error{{use 'template' keyword to treat
'f1' as a dependent template name}}
T::getAs(); // expected-error{{use 'template' keyword to treat 'getAs'
as a dependent template name}}
Index: FixIt/fixit-template-for-dependent-name.cpp
===
--- /dev/null
+++ FixIt/fixit-template-for-dependent-name.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+template void foo() {
+T::bar(); // expected-error {{use 'template' keyword to treat 'bar'
as a dependent template name}}
+T::baz(); // expected-error {{use 'template' keyword to treat 'baz'
as a dependent template name}}
+}
+
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 |
FileCheck %s
+// CHECK: fix-it:{{.*}}:{4:8-4:8}:"template "
+// CHECK: fix-it:{{.*}}:{5:8-5:8}:"template "
Index: Parse/ParseTemplate.cpp
===
--- Parse/ParseTemplate.cpp
+++ Parse/ParseTemplate.cpp
@@ -1247,30 +1247,45 @@
/// template argument list (starting with the '<') and never as a '<'
/// expression.
bool Parser::IsTemplateArgumentList(unsigned Skip) {
- struct AlwaysRevertAction : TentativeParsingAction {
-AlwaysRevertAction(Parser &P) : TentativeParsingAction(P) { }
-~AlwaysRevertAction() { Revert(); }
- } Tentative(*this);
-
- while (Skip) {
-ConsumeAnyToken();
---Skip;
+ {
+RevertingTentativeParsingAction AutoRevertScopeGuard(*this);
+
+for (; Skip > 0; --Skip) {
+ ConsumeToken();
+}
+
+if (!TryConsumeToken(tok::less))
+ return false;
+
+// If the first wannabe template argument consists only of decl specs
+// it's a template argument list indeed.
+
+// See whether we have declaration specifiers, which indicate a type.
+while (isCXXDeclarationSpecifier() == TPResult::True)
+ ConsumeAnyToken();
+
+// If we have a '>' or a ',' then this is a template argument list.
+if (Tok.isOneOf(tok::greater, tok::comma))
+ return true;
}
-
- // '<'
+
+ RevertingTentativeParsingAction AutoRevertScopeGuard(*this);
+
+ for (; Skip > 0; --Skip) {
+ConsumeToken();
+ }
+
if (!TryConsumeToken(tok::less))
return false;
- // An empty template argument list.
- if (Tok.is(tok::greater))
-return true;
-
- // See whether we have
[PATCH] D41306: [clangd] Update documentation page with new features, instructions
jkorous-apple accepted this revision. jkorous-apple added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41306 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41897: Fixing a crash in Sema.
jkorous-apple created this revision.
jkorous-apple added a reviewer: arphaman.
The original code is checking for inaccessible base classes but does not expect
inheriting from template parameters (or dependent types in general) as these
are not modelled by CXXRecord.
Issue was at this line since getAsCXXRecord() returned nullptr:
bool found
= Class->isDerivedFrom(CanonicalBase->getAsCXXRecordDecl(), Paths);
https://reviews.llvm.org/D41897
Files:
Sema/SemaDeclCXX.cpp
SemaCXX/base-class-ambiguity-check.cpp
Index: SemaCXX/base-class-ambiguity-check.cpp
===
--- /dev/null
+++ SemaCXX/base-class-ambiguity-check.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only %s
+
+template
+class Foo
+{
+struct Base : T
+{ };
+
+struct Derived : Base, T
+{ };
+};
\ No newline at end of file
Index: Sema/SemaDeclCXX.cpp
===
--- Sema/SemaDeclCXX.cpp
+++ Sema/SemaDeclCXX.cpp
@@ -2417,9 +2417,17 @@
// Attach the remaining base class specifiers to the derived class.
Class->setBases(Bases.data(), NumGoodBases);
+ // Check that the only base classes that are duplicate are virtual.
for (unsigned idx = 0; idx < NumGoodBases; ++idx) {
// Check whether this direct base is inaccessible due to ambiguity.
QualType BaseType = Bases[idx]->getType();
+
+// Skip all dependent types in templates being used as base specifiers.
+// Checks below assume that base specifier is a CXXRecord.
+if (BaseType->isDependentType()) {
+ continue;
+}
+
CanQualType CanonicalBase = Context.getCanonicalType(BaseType)
.getUnqualifiedType();
Index: SemaCXX/base-class-ambiguity-check.cpp
===
--- /dev/null
+++ SemaCXX/base-class-ambiguity-check.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only %s
+
+template
+class Foo
+{
+struct Base : T
+{ };
+
+struct Derived : Base, T
+{ };
+};
\ No newline at end of file
Index: Sema/SemaDeclCXX.cpp
===
--- Sema/SemaDeclCXX.cpp
+++ Sema/SemaDeclCXX.cpp
@@ -2417,9 +2417,17 @@
// Attach the remaining base class specifiers to the derived class.
Class->setBases(Bases.data(), NumGoodBases);
+ // Check that the only base classes that are duplicate are virtual.
for (unsigned idx = 0; idx < NumGoodBases; ++idx) {
// Check whether this direct base is inaccessible due to ambiguity.
QualType BaseType = Bases[idx]->getType();
+
+// Skip all dependent types in templates being used as base specifiers.
+// Checks below assume that base specifier is a CXXRecord.
+if (BaseType->isDependentType()) {
+ continue;
+}
+
CanQualType CanonicalBase = Context.getCanonicalType(BaseType)
.getUnqualifiedType();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41897: Fixing a crash in Sema.
jkorous-apple updated this revision to Diff 129284.
jkorous-apple added a comment.
Changes based on Aaron's feedback.
https://reviews.llvm.org/D41897
Files:
Sema/SemaDeclCXX.cpp
SemaCXX/base-class-ambiguity-check.cpp
Index: SemaCXX/base-class-ambiguity-check.cpp
===
--- /dev/null
+++ SemaCXX/base-class-ambiguity-check.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only %s
+
+template class Foo {
+ struct Base : T {};
+
+ // Up to commit 680b3a8619 (2018-01-10) this produced a crash in Sema.
+ struct Derived : Base, T {};
+};
Index: Sema/SemaDeclCXX.cpp
===
--- Sema/SemaDeclCXX.cpp
+++ Sema/SemaDeclCXX.cpp
@@ -2417,9 +2417,16 @@
// Attach the remaining base class specifiers to the derived class.
Class->setBases(Bases.data(), NumGoodBases);
+ // Check that the only base classes that are duplicate are virtual.
for (unsigned idx = 0; idx < NumGoodBases; ++idx) {
// Check whether this direct base is inaccessible due to ambiguity.
QualType BaseType = Bases[idx]->getType();
+
+// Skip all dependent types in templates being used as base specifiers.
+// Checks below assume that the base specifier is a CXXRecord.
+if (BaseType->isDependentType())
+ continue;
+
CanQualType CanonicalBase = Context.getCanonicalType(BaseType)
.getUnqualifiedType();
Index: SemaCXX/base-class-ambiguity-check.cpp
===
--- /dev/null
+++ SemaCXX/base-class-ambiguity-check.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only %s
+
+template class Foo {
+ struct Base : T {};
+
+ // Up to commit 680b3a8619 (2018-01-10) this produced a crash in Sema.
+ struct Derived : Base, T {};
+};
Index: Sema/SemaDeclCXX.cpp
===
--- Sema/SemaDeclCXX.cpp
+++ Sema/SemaDeclCXX.cpp
@@ -2417,9 +2417,16 @@
// Attach the remaining base class specifiers to the derived class.
Class->setBases(Bases.data(), NumGoodBases);
+ // Check that the only base classes that are duplicate are virtual.
for (unsigned idx = 0; idx < NumGoodBases; ++idx) {
// Check whether this direct base is inaccessible due to ambiguity.
QualType BaseType = Bases[idx]->getType();
+
+// Skip all dependent types in templates being used as base specifiers.
+// Checks below assume that the base specifier is a CXXRecord.
+if (BaseType->isDependentType())
+ continue;
+
CanQualType CanonicalBase = Context.getCanonicalType(BaseType)
.getUnqualifiedType();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42938: [Sema] Emit -Winteger-overflow for arguments in function calls, ObjC messages.
jkorous-apple added a comment. Maybe a stupid idea but in case it makes sense to add these expression types could we also add integer template arguments? https://reviews.llvm.org/D42938 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42938: [Sema] Emit -Winteger-overflow for arguments in function calls, ObjC messages.
jkorous-apple added a comment.
Sorry, my bad, I tried just older clang version which didn't produce any error.
With reasonably up to date clang I get these:
> cat tmpl_overflow.hpp
template struct foo { short a; };
template<> struct foo<100 * 10240> { bool a; };
int main() {
foo<100 * 10240> a;
}
> ~/src/oss/llvm@master/build/bin/clang++ tmpl_overflow.hpp
tmpl_overflow.hpp:3:23: error: non-type template argument is not a constant
expression
template<> struct foo<100 * 10240> { bool a; };
^~~
tmpl_overflow.hpp:3:43: note: value 1024000 is outside the
range of representable values of type 'long'
template<> struct foo<100 * 10240> { bool a; };
^
tmpl_overflow.hpp:6:9: error: non-type template argument is not a constant
expression
foo<100 * 10240> a;
^~~
tmpl_overflow.hpp:6:29: note: value 1024000 is outside the
range of representable values of type 'long'
foo<100 * 10240> a;
^
2 errors generated.
I am not actually sure those error messages are correct (not constant? type
'long'?) but I am not sure it's still within the scope of your patch either.
What is interesting is that by using lower value (which is still to big for
short) I probably hit different diagnostic check and get different error:
> cat tmpl_overflow.hpp
template struct foo { short a; };
template<> struct foo<100 * 10240> { bool a; };
int main() {
foo<100 * 10240> a;
}
jankorous @ jans-imac /Users/jankorous/tmp
> ~/src/oss/llvm@master/build/bin/clang++ tmpl_overflow.hpp
tmpl_overflow.hpp:3:23: error: non-type template argument evaluates to
1024000, which cannot be narrowed to type 'int' [-Wc++11-narrowing]
template<> struct foo<100 * 10240> { bool a; };
^
tmpl_overflow.hpp:6:9: error: non-type template argument evaluates to
1024000, which cannot be narrowed to type 'int' [-Wc++11-narrowing]
foo<100 * 10240> a;
^
2 errors generated.
https://reviews.llvm.org/D42938
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44088: [clangd] Extract ClangdServer::Options struct.
jkorous-apple added inline comments. Comment at: clang-tools-extra/trunk/clangd/ClangdServer.h:131 + // Features like indexing must be enabled if desired. + static Options optsForTest(); + Shouldn't this be better implemented as a utility function in tests? Also what is the purpose of making global function in header file static? Repository: rL LLVM https://reviews.llvm.org/D44088 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44575: [clangd][nfc] Give name to a magic constant
jkorous-apple created this revision.
jkorous-apple added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, ioeric, ilya-biryukov.
Since I was reading this code I decided I might just as well polish it a
little. It is just preliminary commit for a bug-fix.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44575
Files:
clangd/index/Index.h
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -54,16 +54,18 @@
}
private:
+ static constexpr unsigned HashByteLength = 20;
+
friend llvm::hash_code hash_value(const SymbolID &ID) {
// We already have a good hash, just return the first bytes.
-static_assert(sizeof(size_t) <= 20, "size_t longer than SHA1!");
+static_assert(sizeof(size_t) <= HashByteLength, "size_t longer than
SHA1!");
return *reinterpret_cast(ID.HashValue.data());
}
friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const SymbolID &ID);
friend void operator>>(llvm::StringRef Str, SymbolID &ID);
- std::array HashValue;
+ std::array HashValue;
};
// Write SymbolID into the given stream. SymbolID is encoded as a 40-bytes
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -54,16 +54,18 @@
}
private:
+ static constexpr unsigned HashByteLength = 20;
+
friend llvm::hash_code hash_value(const SymbolID &ID) {
// We already have a good hash, just return the first bytes.
-static_assert(sizeof(size_t) <= 20, "size_t longer than SHA1!");
+static_assert(sizeof(size_t) <= HashByteLength, "size_t longer than SHA1!");
return *reinterpret_cast(ID.HashValue.data());
}
friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const SymbolID &ID);
friend void operator>>(llvm::StringRef Str, SymbolID &ID);
- std::array HashValue;
+ std::array HashValue;
};
// Write SymbolID into the given stream. SymbolID is encoded as a 40-bytes
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44576: [clangd] Fix undefined behavior due to misaligned type cast
jkorous-apple created this revision.
jkorous-apple added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, ioeric, ilya-biryukov.
The current code is casting pointer to a misaligned type which is undefined
behavior.
Found by compiling with Undefined Behavior Sanitizer and running tests
(check-clang-tools).
AFAIK clang produces the same code for reinterpret_cast<>() and memcpy().
Follow-up to: https://reviews.llvm.org/D44575
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44576
Files:
clangd/index/Index.h
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -59,7 +59,9 @@
friend llvm::hash_code hash_value(const SymbolID &ID) {
// We already have a good hash, just return the first bytes.
static_assert(sizeof(size_t) <= HashByteLength, "size_t longer than
SHA1!");
-return *reinterpret_cast(ID.HashValue.data());
+size_t Result;
+memcpy(&Result, ID.HashValue.data(), sizeof(size_t));
+return llvm::hash_code(Result);
}
friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const SymbolID &ID);
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -59,7 +59,9 @@
friend llvm::hash_code hash_value(const SymbolID &ID) {
// We already have a good hash, just return the first bytes.
static_assert(sizeof(size_t) <= HashByteLength, "size_t longer than SHA1!");
-return *reinterpret_cast(ID.HashValue.data());
+size_t Result;
+memcpy(&Result, ID.HashValue.data(), sizeof(size_t));
+return llvm::hash_code(Result);
}
friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const SymbolID &ID);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36918: [Sema] Take into account the current context when checking the accessibility of a member function pointer
jkorous added a comment. I don't particularly like that between setting the DeclContext (SemaTemplateDeduction.cpp:3814) and actually using it (CheckAccess() in SemaAccess.cpp:1459) are some 20 stack frames but it looks like you already tried fixing this "locally" in your initial approach. I assume we can't get the appropriate DeclContext from Expr *OvlExpr in CheckAddressOfMemberAccess(), right? Repository: rC Clang https://reviews.llvm.org/D36918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D36918: [Sema] Take into account the current context when checking the accessibility of a member function pointer
jkorous accepted this revision. jkorous added a comment. LGTM. Thank you for the explanation! Repository: rC Clang https://reviews.llvm.org/D36918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45884: [Sema] Fix parsing of anonymous union in language linkage specification
This revision was automatically updated to reflect the committed changes.
Closed by commit rC334062: [Sema] Fix parsing of anonymous union in language
linkage specification (authored by jkorous, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D45884?vs=143736&id=150069#toc
Repository:
rC Clang
https://reviews.llvm.org/D45884
Files:
lib/Sema/SemaDecl.cpp
test/SemaCXX/anonymous-union-export.cpp
test/SemaCXX/anonymous-union.cpp
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -4642,12 +4642,14 @@
unsigned DiagID;
if (Record->isUnion()) {
// C++ [class.union]p6:
+ // C++17 [class.union.anon]p2:
// Anonymous unions declared in a named namespace or in the
// global namespace shall be declared static.
+ DeclContext *OwnerScope = Owner->getRedeclContext();
if (DS.getStorageClassSpec() != DeclSpec::SCS_static &&
- (isa(Owner) ||
- (isa(Owner) &&
-cast(Owner)->getDeclName( {
+ (OwnerScope->isTranslationUnit() ||
+ (OwnerScope->isNamespace() &&
+!cast(OwnerScope)->isAnonymousNamespace( {
Diag(Record->getLocation(), diag::err_anonymous_union_not_static)
<< FixItHint::CreateInsertion(Record->getLocation(), "static ");
Index: test/SemaCXX/anonymous-union.cpp
===
--- test/SemaCXX/anonymous-union.cpp
+++ test/SemaCXX/anonymous-union.cpp
@@ -80,6 +80,10 @@
float float_val;
};
+extern "C++" {
+union { }; // expected-error{{anonymous unions at namespace or global scope
must be declared 'static'}}
+}
+
static union {
int int_val2; // expected-note{{previous definition is here}}
float float_val2;
Index: test/SemaCXX/anonymous-union-export.cpp
===
--- test/SemaCXX/anonymous-union-export.cpp
+++ test/SemaCXX/anonymous-union-export.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -std=c++17 -fmodules-ts -emit-obj -verify %s
+
+export module M;
+export {
+union { bool a; }; // expected-error{{anonymous unions at namespace or
global scope must be declared 'static'}}
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -4642,12 +4642,14 @@
unsigned DiagID;
if (Record->isUnion()) {
// C++ [class.union]p6:
+ // C++17 [class.union.anon]p2:
// Anonymous unions declared in a named namespace or in the
// global namespace shall be declared static.
+ DeclContext *OwnerScope = Owner->getRedeclContext();
if (DS.getStorageClassSpec() != DeclSpec::SCS_static &&
- (isa(Owner) ||
- (isa(Owner) &&
-cast(Owner)->getDeclName( {
+ (OwnerScope->isTranslationUnit() ||
+ (OwnerScope->isNamespace() &&
+!cast(OwnerScope)->isAnonymousNamespace( {
Diag(Record->getLocation(), diag::err_anonymous_union_not_static)
<< FixItHint::CreateInsertion(Record->getLocation(), "static ");
Index: test/SemaCXX/anonymous-union.cpp
===
--- test/SemaCXX/anonymous-union.cpp
+++ test/SemaCXX/anonymous-union.cpp
@@ -80,6 +80,10 @@
float float_val;
};
+extern "C++" {
+union { }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+}
+
static union {
int int_val2; // expected-note{{previous definition is here}}
float float_val2;
Index: test/SemaCXX/anonymous-union-export.cpp
===
--- test/SemaCXX/anonymous-union-export.cpp
+++ test/SemaCXX/anonymous-union-export.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -std=c++17 -fmodules-ts -emit-obj -verify %s
+
+export module M;
+export {
+union { bool a; }; // expected-error{{anonymous unions at namespace or global scope must be declared 'static'}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53290: [WIP] clangd Transport layer
jkorous created this revision.
jkorous added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, dexonsmith,
MaskRay, ioeric, ilya-biryukov, mgorny.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53290
Files:
clangd/CMakeLists.txt
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/JSONRPCDispatcher.cpp
clangd/JSONRPCDispatcher.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/ProtocolHandlers.h
clangd/Transport.cpp
clangd/Transport.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -315,13 +315,20 @@
CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
CCOpts.AllScopes = AllScopesCompletion;
+ // TODO
+ std::unique_ptr TransportLayer = newJSONTransport(stdin, llvm::outs(), InputStyle);
+ if (!TransportLayer.get()) {
+ // TODO
+ return 42;
+ }
+
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(
- Out, CCOpts, CompileCommandsDirPath,
+ *TransportLayer.get(), CCOpts, CompileCommandsDirPath,
/*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
constexpr int NoShutdownRequestErrorCode = 1;
llvm::set_thread_name("clangd.main");
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
- return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
+ return LSPServer.run() ? 0 : NoShutdownRequestErrorCode;
}
Index: clangd/Transport.h
===
--- /dev/null
+++ clangd/Transport.h
@@ -0,0 +1,94 @@
+//===--- Transport.h - sending and receiving LSP messages ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// The language server protocol is usually implemented by writing messages as
+// JSON-RPC over the stdin/stdout of a subprocess. However other communications
+// mechanisms are possible, such as XPC on mac (see xpc/ directory).
+//
+// The Transport interface allows the mechanism to be replaced, and the JSONRPC
+// Transport is the standard implementation.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TRANSPORT_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TRANSPORT_H_
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace clang {
+namespace clangd {
+
+// A transport is responsible for maintaining the connection to a client
+// application, and reading/writing structured messages to it.
+//
+// Transports have limited thread safety requirements:
+// - messages will not be sent concurrently
+// - messages MAY be sent while loop() is reading, or its callback is active
+class Transport {
+public:
+ Transport() : shouldTerminateLoop(false) { }
+ virtual ~Transport() = default;
+
+ // Called by Clangd to send messages to the client.
+ // (Because JSON and XPC are so similar, these are concrete and delegate to
+ // sendMessage. We could change this to support more diverse transports).
+ void notifyClient(llvm::StringRef Method, llvm::json::Value Params);
+ void callClient(llvm::StringRef Method, llvm::json::Value Params,
+llvm::json::Value ID);
+ void replyToClient(llvm::json::Value ID, llvm::Expected Result);
+
+ // Implemented by Clangd to handle incoming messages. (See loop() below).
+ class MessageHandler {
+ public:
+// TODO was originally abstract
+virtual ~MessageHandler() {}
+virtual bool notifyServer(llvm::StringRef Method, llvm::json::Value ) = 0;
+virtual bool callServer(llvm::StringRef Method, llvm::json::Value Params,
+ llvm::json::Value ID) = 0;
+virtual bool replyToServer(llvm::json::Value ID,
+ llvm::Expected Result) = 0;
+ };
+ // Called by Clangd to receive messages from the client.
+ // The transport should in turn invoke the handler to process messages.
+ // If handler returns true, the transport should immedately return success.
+ // Otherwise, it returns an error when the transport becomes unusable.
+ // (Because JSON and XPC are so similar, they share handleMessage()).
+ virtual bool loop(MessageHandler &) = 0;
+
+ std::atomic shouldTerminateLoop;
+
+protected:
+ // Common implementation for notify(), call(), and reply().
+ virtual void sendMessage(llvm::json::Value) = 0;
+ // Delegates to notify(), call(), and reply().
+ bool handleMessage(llvm::json::Value, MessageHandler&);
+};
+
+// Controls the way JSON-RPC messages are encoded (both input and output).
+enum JSONSt
[PATCH] D56611: [clangd] A code action to swap branches of an if statement
jkorous added a comment.
Hi Ilya, this seems really useful for people learning how to implement their
custom actions!
Comment at: clangd/refactor/actions/SwapIfBranches.cpp:36
+
+ bool VisitIfStmt(IfStmt *If) {
+auto R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
It seems to me we don't find If token whenever
CursorLoc == location of 'f' of the If token
For example if there's "if" starting at location 1:1 I think we proceed like
this (hope my pseudocode is clear):
1. `If->getIfLoc()` returns `SourceLocation{1:1}`
2. we construct `SourceRange{begin = 1:1, end = 1:1}`
3. `toHalfOpenFileRange()` returns `SourceRange{1:1, 1:2}`
4. the condition for `SourceLocation L` in `halfOpenRangeContains()` is `1:1 <=
LOffset && LOffset < 1:2` which is only ever true for L == 1:1
Do I understand it right?
Comment at: clangd/refactor/actions/SwapIfBranches.cpp:62
+
+ // avoid dealing with single-statement brances, they require careful handling
+ // to avoid changing semantics of the code (i.e. dangling else).
Just a typo in comment `s/brances/branches/`
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56611/new/
https://reviews.llvm.org/D56611
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56610: [clangd] A code action to qualify an unqualified name
jkorous added a comment. Hi Ilya, I got here from reading your other patch (https://reviews.llvm.org/D56611). I'm wondering if we could make those range utility functions more understandable. Comment at: clangd/SourceCode.h:64 +/// Turns a token range into a half-open range and checks its correctness. +/// The resulting range will have only valid source location on both sides, both It seems to me the range is actually closed on both sides. Do I get it right that the result is? ``` [begin of first token : end of last token] ``` Wouldn't some name like `toWholeTokenRange` be easier to understand? Comment at: clangd/SourceCode.h:81 +/// Preconditions: isValidFileRange(R) == true, L is a file location. +bool halfOpenRangeContains(const SourceManager &Mgr, SourceRange R, + SourceLocation L); I'd find it helpful to mention that the range is actually interpreted as closed-open (knowing which half is which). Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56610/new/ https://reviews.llvm.org/D56610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54428: [clangd] XPC transport layer, framework, test-client
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE351280: [clangd] XPC transport layer (authored by jkorous,
committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54428?vs=180809&id=181925#toc
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54428/new/
https://reviews.llvm.org/D54428
Files:
CMakeLists.txt
clangd/CMakeLists.txt
clangd/Features.inc.in
clangd/Transport.h
clangd/tool/CMakeLists.txt
clangd/tool/ClangdMain.cpp
clangd/xpc/CMakeLists.txt
clangd/xpc/Conversion.cpp
clangd/xpc/Conversion.h
clangd/xpc/README.txt
clangd/xpc/XPCTransport.cpp
clangd/xpc/cmake/Info.plist.in
clangd/xpc/cmake/XPCServiceInfo.plist.in
clangd/xpc/cmake/modules/CreateClangdXPCFramework.cmake
clangd/xpc/framework/CMakeLists.txt
clangd/xpc/framework/ClangdXPC.cpp
clangd/xpc/test-client/CMakeLists.txt
clangd/xpc/test-client/ClangdXPCTestClient.cpp
test/CMakeLists.txt
test/clangd/xpc/initialize.test
test/lit.cfg
test/lit.site.cfg.in
unittests/clangd/CMakeLists.txt
unittests/clangd/xpc/CMakeLists.txt
unittests/clangd/xpc/ConversionTests.cpp
Index: unittests/clangd/xpc/ConversionTests.cpp
===
--- unittests/clangd/xpc/ConversionTests.cpp
+++ unittests/clangd/xpc/ConversionTests.cpp
@@ -0,0 +1,36 @@
+//===-- ConversionTests.cpp --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/Conversion.h"
+#include "gtest/gtest.h"
+
+#include
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+TEST(JsonXpcConversionTest, JsonToXpcToJson) {
+
+ for (auto &testcase :
+ {json::Value(false), json::Value(3.14), json::Value(42),
+json::Value(-100), json::Value("foo"), json::Value(""),
+json::Value("123"), json::Value(" "),
+json::Value{true, "foo", nullptr, 42},
+json::Value(json::Object{
+{"a", true}, {"b", "foo"}, {"c", nullptr}, {"d", 42}})}) {
+EXPECT_TRUE(testcase == xpcToJson(jsonToXpc(testcase))) << testcase;
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/xpc/CMakeLists.txt
===
--- unittests/clangd/xpc/CMakeLists.txt
+++ unittests/clangd/xpc/CMakeLists.txt
@@ -0,0 +1,21 @@
+set(LLVM_LINK_COMPONENTS
+ support
+ )
+
+get_filename_component(CLANGD_SOURCE_DIR
+ ${CMAKE_CURRENT_SOURCE_DIR}/../../clangd REALPATH)
+include_directories(
+ ${CLANGD_SOURCE_DIR}
+ )
+
+add_extra_unittest(ClangdXpcTests
+ ConversionTests.cpp
+ )
+
+target_link_libraries(ClangdXpcTests
+ PRIVATE
+ clangdXpcJsonConversions
+ clangDaemon
+ LLVMSupport
+ LLVMTestingSupport
+ )
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -65,3 +65,7 @@
LLVMSupport
LLVMTestingSupport
)
+
+if (CLANGD_BUILD_XPC)
+ add_subdirectory(xpc)
+endif ()
Index: clangd/Features.inc.in
===
--- clangd/Features.inc.in
+++ clangd/Features.inc.in
@@ -0,0 +1 @@
+#define CLANGD_BUILD_XPC @CLANGD_BUILD_XPC@
Index: clangd/xpc/framework/ClangdXPC.cpp
===
--- clangd/xpc/framework/ClangdXPC.cpp
+++ clangd/xpc/framework/ClangdXPC.cpp
@@ -0,0 +1,5 @@
+
+/// Returns the bundle identifier of the Clangd XPC service.
+extern "C" const char *clangd_xpc_get_bundle_identifier() {
+ return "org.llvm.clangd";
+}
Index: clangd/xpc/framework/CMakeLists.txt
===
--- clangd/xpc/framework/CMakeLists.txt
+++ clangd/xpc/framework/CMakeLists.txt
@@ -0,0 +1,9 @@
+
+set(SOURCES
+ClangdXPC.cpp)
+add_clang_library(ClangdXPCLib SHARED
+ ${SOURCES}
+ DEPENDS
+ clangd
+)
+create_clangd_xpc_framework(ClangdXPCLib "ClangdXPC")
Index: clangd/xpc/CMakeLists.txt
===
--- clangd/xpc/CMakeLists.txt
+++ clangd/xpc/CMakeLists.txt
@@ -0,0 +1,29 @@
+set(CLANGD_XPC_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}")
+set(CLANGD_XPC_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}")
+
+list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")
+include(CreateClangdXPCFramework)
+
+add_subdirectory(framework)
+add_subdirectory(test-client)
+
+include_directories(
+ ${CMAKE_CURRENT_SOURCE_DIR}/../
+)
+
+set(LLVM_LINK_COMPONENTS
+ Support
+ )
+
+# Needed by LLVM's CMake checks because this file defines multiple targets.
+set(LLVM_OPTIONAL_SOURCES Conversio
[PATCH] D50452: [WIP] clangd XPC adapter
jkorous added a comment. Abandonned in favor of https://reviews.llvm.org/D54428 Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50452/new/ https://reviews.llvm.org/D50452 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D56561: [Preprocessor] For missing file in framework add note about framework location.
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM with nits about doxygen annotations. Comment at: clang/include/clang/Lex/DirectoryLookup.h:175 + /// \param [out] IsFrameworkFound For a framework directory set to true if + /// specified '.framework' directory is found. + /// We might make this easier to understand by explaining where/how the '.framework' directory is specified. Comment at: clang/include/clang/Lex/HeaderSearch.h:396 + /// \param IsFrameworkFound If non-null, will be set to true for a framework + /// include and if corresponding '.framework' directory was found. Doesn't + /// guarantee the requested file is found. We might try to explain what exactly is meant by corresponding .framework directory. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56561/new/ https://reviews.llvm.org/D56561 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57057: [clangd] Log clang-tidy configuration, NFC
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57057/new/ https://reviews.llvm.org/D57057 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57228: [clangd] Make USRs for macros to be position independent
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. Thanks for fixing this Kadir! LGTM Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57228/new/ https://reviews.llvm.org/D57228 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one
jkorous requested review of this revision. jkorous added a comment. I tried to move the getNearestOption() to it's only client - EmitUnknownDiagWarning() but it turned out to be a significant change because of clang/Basic/DiagnosticGroups.inc use in DiagnosticIDs.cpp. I suggest we leave that for eventual future refactoring since it would blow up scope of this patch a lot. Repository: rC Clang https://reviews.llvm.org/D49736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM and it seems like all of Eric's comments were answered too. Repository: rC Clang https://reviews.llvm.org/D50926 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50801: [rename] Use isPointWithin when looking for a declaration at location
jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM Thanks for fixing this! Repository: rC Clang https://reviews.llvm.org/D50801 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52554: [WIP] [clangd] Tests for special methods code-completion
jkorous marked 6 inline comments as done.
jkorous added inline comments.
Comment at: clangd/CodeCompleteTests.cpp:2043
+TEST(CompletionTestNoExplicitMembers, Struct) {
+ clangd::CodeCompleteOptions Opts;
sammccall wrote:
> sammccall wrote:
> > (Should this be ImplicitMembers?)
> nit: these cases should probably be moved up with the other code completion
> ones, and called something like `TEST(CompletionTest, NoExplicitMembers)` or
> so for consistency.
>
> It's OK to have related tests in one test case.
>
> In fact, this large set of closely-related cases seems like a good place for
> Go-style table-driven tests.
Ultimately got rid of the name completely.
Comment at: clangd/CodeCompleteTests.cpp:2043
+TEST(CompletionTestNoExplicitMembers, Struct) {
+ clangd::CodeCompleteOptions Opts;
jkorous wrote:
> sammccall wrote:
> > sammccall wrote:
> > > (Should this be ImplicitMembers?)
> > nit: these cases should probably be moved up with the other code completion
> > ones, and called something like `TEST(CompletionTest, NoExplicitMembers)`
> > or so for consistency.
> >
> > It's OK to have related tests in one test case.
> >
> > In fact, this large set of closely-related cases seems like a good place
> > for Go-style table-driven tests.
> Ultimately got rid of the name completely.
I reconsidered the simple yet verbatim approach.
Comment at: clangd/CodeCompleteTests.cpp:2054
+
+ EXPECT_TRUE(ResultsDot.Completions.empty());
+
sammccall wrote:
> EXPECT_THAT(ResultsDot.Completions, IsEmpty())
>
> (when the assertion fails, the failure message will print the contents of the
> container)
I didn't know this. It's pretty neat. Thanks!
Comment at: clangd/CodeCompleteTests.cpp:2139
+
+TEST(CompletionTestMethodDeclared, Struct) {
+ clangd::CodeCompleteOptions Opts;
sammccall wrote:
> doesn't this test a strict superset of what CompletionTestNoTestMembers tests?
> i.e. this also asserts that the implicit members are not included.
>
> ISTM we could combine many of these tests (and that in fact many of them are
> covered by TestAfterDotCompletion.
You are right, I pruned the list of testcases a bit.
Comment at: clangd/CodeCompleteTests.cpp:2338
+
+TEST(CompletionTestSpecialMethodsDeclared,
ExplicitStructTemplateSpecialization) {
+ clangd::CodeCompleteOptions Opts;
sammccall wrote:
> (I think we could get away with a representative set of cases, rather than
> testing the intersection of every feature. e.g. test an explicitly declared
> operator= on a struct, but combining that with an explicitly specialized
> struct template seems unneccesary)
I simplified the whole test and removed some cases that were not really unique.
Do you think the rest is still too exhaustive? (Asking before deleting.)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52554
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52554: [WIP] [clangd] Tests for special methods code-completion
jkorous updated this revision to Diff 172951.
jkorous marked 4 inline comments as done.
jkorous added a comment.
Rewritten tests to shared implementation different cases.
https://reviews.llvm.org/D52554
Files:
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2204,6 +2204,115 @@
{cls("naber"), cls("nx::naber")}, Opts);
EXPECT_THAT(Results.Completions, UnorderedElementsAre());
}
+
+TEST(CompletionTest, NoCompletionForSpecialMembers) {
+ clangd::CodeCompleteOptions Opts;
+ for( auto& testcase : {
+R"cpp(
+ struct foo {};
+ void bar() {
+foo a;
+a.^
+)cpp",
+R"cpp(
+ template struct foo {};
+ void bar() {
+foo a;
+a.^
+)cpp",
+R"cpp(
+template struct foo {}; template<> struct foo {};
+void bar() {
+ foo a;
+ a.^
+)cpp",
+R"cpp(
+ struct foo {};
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ template struct foo {};
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ template struct foo {}; template<> struct foo {};
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ struct foo {};
+ foo::^
+)cpp",
+R"cpp(
+ template struct foo {};
+ foo::^
+)cpp",
+R"cpp(
+ template struct foo {}; template<> struct foo {};
+ foo::^
+)cpp",
+R"cpp(
+ struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo a;
+a.^
+)cpp",
+R"cpp(
+ struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ foo::^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo a;
+a.^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ foo::^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ template<> struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo a;
+a.^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ template<> struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ void bar() {
+foo* b;
+b->^
+)cpp",
+R"cpp(
+ template struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ template<> struct foo { foo(); ~foo(); foo& operator=(const foo&); foo& operator=(foo&&); };
+ foo::^
+)cpp"
+ }) {
+auto Results = completions(testcase, /*IndexSymbols=*/{}, Opts);
+EXPECT_THAT(Results.Completions, IsEmpty()) << testcase;
+ }
+}
+
} // namespace
} // namespace clangd
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54428: [clangd] XPC transport layer, framework, test-client
jkorous created this revision.
jkorous added reviewers: arphaman, sammccall.
Herald added subscribers: cfe-commits, kadircet, dexonsmith, MaskRay, ioeric,
ilya-biryukov, mgorny.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54428
Files:
CMakeLists.txt
Features.inc.in
clangd/CMakeLists.txt
clangd/xpc/CMakeLists.txt
clangd/xpc/ConversionTests.cpp
clangd/xpc/initialize.test
lit.cfg
lit.site.cfg.in
tool/CMakeLists.txt
tool/ClangdMain.cpp
xpc/CMakeLists.txt
xpc/Conversion.cpp
xpc/Conversion.h
xpc/README.txt
xpc/XPCTransport.cpp
xpc/XPCTransport.h
xpc/cmake/Info.plist.in
xpc/cmake/XPCServiceInfo.plist.in
xpc/cmake/modules/CreateClangdXPCFramework.cmake
xpc/framework/CMakeLists.txt
xpc/framework/ClangdXPC.cpp
xpc/test-client/CMakeLists.txt
xpc/test-client/ClangdXPCTestClient.cpp
Index: clangd/xpc/ConversionTests.cpp
===
--- /dev/null
+++ clangd/xpc/ConversionTests.cpp
@@ -0,0 +1,36 @@
+//===-- ConversionTests.cpp --*- C++ -*---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "xpc/Conversion.h"
+#include "gtest/gtest.h"
+
+#include
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using namespace llvm;
+
+TEST(JsonXpcConversionTest, JsonToXpcToJson) {
+
+ for (auto &testcase :
+ {json::Value(false), json::Value(3.14), json::Value(42),
+json::Value(-100), json::Value("foo"), json::Value(""),
+json::Value("123"), json::Value(" "),
+json::Value{true, "foo", nullptr, 42},
+json::Value(json::Object{
+{"a", true}, {"b", "foo"}, {"c", nullptr}, {"d", 42}})}) {
+EXPECT_TRUE(testcase == xpcToJson(jsonToXpc(testcase))) << testcase;
+ }
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clangd/xpc/CMakeLists.txt
===
--- /dev/null
+++ clangd/xpc/CMakeLists.txt
@@ -0,0 +1,21 @@
+set(LLVM_LINK_COMPONENTS
+ support
+ )
+
+get_filename_component(CLANGD_SOURCE_DIR
+ ${CMAKE_CURRENT_SOURCE_DIR}/../../clangd REALPATH)
+include_directories(
+ ${CLANGD_SOURCE_DIR}
+ )
+
+add_extra_unittest(ClangdXpcTests
+ ConversionTests.cpp
+ )
+
+target_link_libraries(ClangdXpcTests
+ PRIVATE
+ clangdXpcJsonConversions
+ clangDaemon
+ LLVMSupport
+ LLVMTestingSupport
+ )
Index: clangd/CMakeLists.txt
===
--- clangd/CMakeLists.txt
+++ clangd/CMakeLists.txt
@@ -60,3 +60,7 @@
LLVMSupport
LLVMTestingSupport
)
+
+if (CLANGD_BUILD_XPC)
+ add_subdirectory(xpc)
+endif ()
Index: lit.site.cfg.in
===
--- lit.site.cfg.in
+++ lit.site.cfg.in
@@ -11,6 +11,7 @@
config.python_executable = "@PYTHON_EXECUTABLE@"
config.target_triple = "@TARGET_TRIPLE@"
config.clang_staticanalyzer = @CLANG_ENABLE_STATIC_ANALYZER@
+config.clangd_xpc_support = @CLANGD_BUILD_XPC_SUPPORT@
# Support substitution of the tools and libs dirs with user parameters. This is
# used when we can't determine the tool dir at configuration time.
Index: lit.cfg
===
--- lit.cfg
+++ lit.cfg
@@ -117,6 +117,10 @@
if platform.system() not in ['Windows']:
config.available_features.add('ansi-escape-sequences')
+# XPC support for Clangd.
+if config.clangd_xpc_support:
+config.available_features.add('clangd-xpc-support')
+
if config.clang_staticanalyzer:
config.available_features.add('static-analyzer')
Index: clangd/xpc/initialize.test
===
--- /dev/null
+++ clangd/xpc/initialize.test
@@ -0,0 +1,10 @@
+# RUN: clangd-xpc-test-client < %s | FileCheck %s
+# REQUIRES: clangd-xpc-support
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
+# CHECK: {"id":0,"jsonrpc":"2.0","result":{"capabilities"
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+# CHECK: {"id":3,"jsonrpc":"2.0","result":null}
+
+{"jsonrpc":"2.0","method":"exit"}
Index: xpc/test-client/ClangdXPCTestClient.cpp
===
--- /dev/null
+++ xpc/test-client/ClangdXPCTestClient.cpp
@@ -0,0 +1,96 @@
+#include "xpc/Conversion.h"
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/LineIterator.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
+#include
+#include
+#include
+#include
+
+typedef const char *(*clangd_xpc_get_bundle_identifier_t)(void);
+
+using namespace llvm;
+
[PATCH] D54529: [clangd] Add USR to textDocument/definition response
jkorous created this revision.
jkorous added reviewers: sammccall, ilya-biryukov, arphaman, benlangmuir.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, kadircet, dexonsmith, MaskRay, ioeric.
We need a way for given code position to get the best definition/declaration
location in given translation unit and it's USR.
Since the first element in list returned by textDocument/definition is meeting
the def/decl location requirement we'd just need to add the USR. Other option
would be to split this to a separate method. I thought this patch to be a
better fit from protocol perspective but I am happy to go the other way if
that's preferred.
I'd like to ask if someone more familiar with gtest matchers could take a look
at changes I made in unit-tests. I am not sure it's exactly the idiomatic way.
I was trying to use Property() matchers first but gave up since I wasn't able
to debug it (got lost in template error messages).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54529
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
ClangdServer.cpp
ClangdServer.h
Protocol.cpp
Protocol.h
XRefs.cpp
XRefs.h
clangd/ClangdTests.cpp
clangd/SyncAPI.cpp
clangd/SyncAPI.h
clangd/XRefsTests.cpp
clangd/textdocument-didchange-fail.test
clangd/xrefs.test
Index: clangd/XRefsTests.cpp
===
--- clangd/XRefsTests.cpp
+++ clangd/XRefsTests.cpp
@@ -94,6 +94,11 @@
}
MATCHER_P(RangeIs, R, "") { return arg.range == R; }
+MATCHER_P(RangeInLocationIs, R, "") { return arg.DefOrDeclLocation.range == R; }
+MATCHER_P(RangeInLocationIsRH, R, "") {
+ return arg == R.DefOrDeclLocation.range;
+}
+MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; }
TEST(GoToDefinition, WithIndex) {
Annotations SymbolHeader(R"cpp(
@@ -124,35 +129,39 @@
^f1();
}
)cpp");
- EXPECT_THAT(runFindDefinitionsWithIndex(Test),
- testing::ElementsAreArray(
- {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())}));
+ EXPECT_THAT(
+ runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray({RangeInLocationIs(SymbolCpp.range("f1")),
+ RangeInLocationIs(Test.range())}));
Test = Annotations(R"cpp(// definition in AST.
void [[f1]]() {}
int main() {
^f1();
}
)cpp");
- EXPECT_THAT(runFindDefinitionsWithIndex(Test),
- testing::ElementsAreArray(
- {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))}));
+ EXPECT_THAT(
+ runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray({RangeInLocationIs(Test.range()),
+ RangeInLocationIs(SymbolHeader.range("f1"))}));
Test = Annotations(R"cpp(// forward declaration in AST.
class [[Foo]];
F^oo* create();
)cpp");
- EXPECT_THAT(runFindDefinitionsWithIndex(Test),
- testing::ElementsAreArray(
- {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())}));
+ EXPECT_THAT(
+ runFindDefinitionsWithIndex(Test),
+ testing::ElementsAreArray({RangeInLocationIs(SymbolHeader.range("foo")),
+ RangeInLocationIs(Test.range())}));
Test = Annotations(R"cpp(// defintion in AST.
class [[Forward]] {};
F^orward create();
)cpp");
EXPECT_THAT(runFindDefinitionsWithIndex(Test),
testing::ElementsAreArray({
- RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")),
+ RangeInLocationIs(Test.range()),
+ RangeInLocationIs(SymbolHeader.range("forward")),
}));
}
@@ -302,12 +311,13 @@
for (const char *Test : Tests) {
Annotations T(Test);
auto AST = TestTU::withCode(T.code()).build();
-std::vector> ExpectedLocations;
-for (const auto &R : T.ranges())
- ExpectedLocations.push_back(RangeIs(R));
-EXPECT_THAT(findDefinitions(AST, T.point()),
-ElementsAreArray(ExpectedLocations))
-<< Test;
+
+auto definitions = findDefinitions(AST, T.point());
+std::vector> Assertions;
+for (const auto &D : definitions)
+ Assertions.push_back(RangeInLocationIsRH(D));
+
+EXPECT_THAT(T.ranges(), ElementsAreArray(Assertions)) << Test;
}
}
@@ -334,22 +344,27 @@
)cpp");
auto AST = TestTU::withCode(T.code()).build();
EXPECT_THAT(findDefinitions(AST, T.point("1")),
- ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4";
+ ElementsAre(RangeInLocationIs(T.range("str")),
+ RangeInLocationIs(T.range("foo4";
EXPECT_THAT(findDefinitions(AST, T.point("2")),
- ElementsAre(RangeIs(T.range("str";
+ ElementsAre(RangeInLocationIs(T.range("str";
EXPECT_THAT(findDefinitions(AST, T.point("3")),
-
[PATCH] D54529: [clangd] Add USR to textDocument/definition response
jkorous added a comment. One more thing - shall I create new client capability flag for this? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54529: [clangd] Add USR to textDocument/definition response
jkorous added a comment. Hi Sam! I am aware of clangd using SymbolID. We basically need USR for integration with our external indexing service. We don't plan to systematically use it in the protocol and I am not aware of any other requirement for using USR in LSP - will double check that to be sure. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54529: [clangd] Add USR to textDocument/definition response
jkorous added a comment. We are also discussing creating separate method as we'll likely want to add other data to the response in the future. Would you prefer USR not to be in the standard part of LSP but only in our extension? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54529: [clangd] Add USR to textDocument/definition response
jkorous added a comment. Correction - I asked offline about our intended use of USR in LSP and it seems we might want to receive it as part of other responses too. Nothing specific for now but it's probable that it won't be just this singular case. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54529 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53900: [CodeComplete] Penalize inherited ObjC properties for auto-completion
jkorous added reviewers: arphaman, benlangmuir. jkorous added a comment. This looks reasonable to me but asked people with more context to take a look. Repository: rC Clang https://reviews.llvm.org/D53900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54428: [clangd] XPC transport layer, framework, test-client
jkorous planned changes to this revision.
jkorous added a comment.
In https://reviews.llvm.org/D54428#1297147, @sammccall wrote:
> A question about the high-level build target setup (I don't know much about
> XPC services/frameworks, bear with me...):
>
> This is set up so that the clangd binary (ClangdMain) can run unmodified as
> an XPC service, all flags and options are still respected etc.
> At the same time, the framework plist seems like it is designed to invoke
> the clangd binary in a very specific configuration (no flags will be plumbed
> through).
>
> Is it actually important that the `clangd` binary itself can be used with
> XPC, rather than defining another binary that spawns a
> `ClangdServer+XPCTransport` with the right config? If you strip out all of
> `ClangdMain` that's not related to flag parsing and options, there's almost
> nothing left...
I don't really expect you to be familiar with XPC - feel free to ask about
anything and I'll do my best explaining.
It's more like that configuration isn't implemented in this initial patch but
as we can't use command line options (launchd doesn't pass these to XPC
service) we'll have to duplicate these as environment variables. We discusse
internally and we prefer to have just single binary as that would make
integration and testing easier for us.
Comment at: tool/ClangdMain.cpp:30
+#ifdef CLANGD_BUILD_XPC
+#include "xpc/XPCTransport.h"
+#endif
sammccall wrote:
> WDYT about putting `newXPCTransport()` in `Transport.h` behind an ifdef?
>
> That will be consistent with JSON, allow the `XPCTransport.h` to be removed
> entirely, and remove one #ifdef here.
>
> I find the #ifdefs easier to understand if they surround functional code,
> rather than #includes - might be just me.
Ok, sounds reasonable.
Comment at: tool/ClangdMain.cpp:328
+#ifdef CLANGD_BUILD_XPC
+if (getenv("CLANGD_AS_XPC_SERVICE"))
+ return newXPCransport();
sammccall wrote:
> I'd consider putting this outside the #ifdef, so we can print an error and
> exit if it's requested but not built.
>
> In fact, can this just be a regular flag instead? `-transport={xpc|json}`
Unfortunately it's not possible to have launchd pass flags when spawning XPC
services (otherwise it would be my first choice too).
Comment at: tool/ClangdMain.cpp:329
+if (getenv("CLANGD_AS_XPC_SERVICE"))
+ return newXPCransport();
+#endif
sammccall wrote:
> no support for `-input-mirror` or pretty-printing in the logs - deliberate?
I was thinking this could be added later.
Comment at: xpc/CMakeLists.txt:21
+
+add_clang_library(clangdXpcJsonConversions
+ Conversion.cpp
sammccall wrote:
> why is conversions a separate library from transport?
>
> No problem with fine-grained targets per se, but it's inconsistent with much
> of the rest of clang-tools-extra.
Guilty of YAGNI violation here - was thinking about eventual use by XPC
clients. I'll stick to consistency for now and would separate it if needed.
Comment at: xpc/Conversion.cpp:16
+
+using namespace clang;
+using namespace clangd;
sammccall wrote:
> nit:
> ```
> using namespace llvm;
> namespace clang {
> namespace clangd {
> ```
> (we're finally consistent about this)
Ok. Thanks.
Comment at: xpc/Conversion.cpp:22
+xpc_object_t clangd::jsonToXpc(const json::Value &json) {
+ const char *const key = "LSP";
+ std::string payload_string;
sammccall wrote:
> Key, PayloadString, etc
Thanks. Somehow I was always working on projects with different naming
conventions and still struggle with this.
Comment at: xpc/Conversion.cpp:23
+ const char *const key = "LSP";
+ std::string payload_string;
+ raw_string_ostream payload_stream(payload_string);
sammccall wrote:
> nit: you can #include ScopedPrinter, and then this is just
> llvm::to_string(json)
I'll take a look at ScopedPrinter. Thanks.
Comment at: xpc/Conversion.cpp:28
+ xpc_object_t payload_obj = xpc_string_create(payload_string.c_str());
+ return xpc_dictionary_create(&key, &payload_obj, 1);
+}
sammccall wrote:
> ah, this encoding is a little sad vs the "native" encoding of object ->
> xpc_dictionary, array -> xpc_array etc which looked promising...
>
> Totally your call, but out of curiosity - why the change?
I know. At first we didn't even think of anything else than 1:1 mapping between
JSON and XPC dictionary as it's just natural. But later we learned about
performance issues with construction of XPC dictionaries with lots of elements
which would be a problem for code completion.
Comment at: xpc/Conversion.h:19
+
+xpc_object_t jsonToXpc(const llvm::json::Value &json);
+llvm::json::Value xpcToJson(const xpc_object_t &json);
sammccall wrote:
> param nam
[PATCH] D54047: Check TUScope is valid before use
jkorous added a comment. Could you please add a test? I'd suggest minimizing the testcase you linked and placing it to `clang/test`. Repository: rC Clang https://reviews.llvm.org/D54047 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49736: [Basic] Emit warning flag suggestion only in case there's existing flag *similar* to the unknown one
jkorous added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D49736 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53807: Create a diagnostic group for warn_call_to_pure_virtual_member_function_from_ctor_dtor, so it can be turned into an error using Werror
jkorous added a reviewer: jkorous. jkorous accepted this revision. jkorous added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the patch! Repository: rC Clang https://reviews.llvm.org/D53807 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54047: Check TUScope is valid before use
jkorous added reviewers: rjmccall, doug.gregor. jkorous added subscribers: rjmccall, doug.gregor. jkorous added a comment. Adding @rjmccall and @doug.gregor who might have some insight. Honestly, I don't know how IWYU works and my familiarity with Sema is limited so bear with me. From what I see `TUScope` is set only in one place in clang and that is `void Sema::ActOnTranslationUnitScope(Scope *S)`. > grep -nr --exclude="*/test/*" "TUScope\s\+=" . ./lib/Sema/Sema.cpp:70: TUScope = S; ./lib/Sema/Sema.cpp:149: TUScope = nullptr; ./lib/Sema/Sema.cpp:946: TUScope = nullptr; ./lib/Sema/Sema.cpp:1168:TUScope = nullptr; (TUScope seems not to be copied anywhere.) > grep -nr --exclude="*/test/*" "=\s\+TUScope" . `ActOnTranslationUnitScope` itself is called only from `void Parser::Initialize()`. Just an idea - can't your code mimic behavior of `Parser::Initialize()`? Other than that it seems that: 1. The code obviously assumes `TUScope != nullptr`. 2. We aren't aware of hitting this case in clang. So even with my limited insight I would be inclined to make the assumption explicit. The whole block of code after `if (isValidVarArgType(E->getType()) == VAK_Undefined)` seems to be mostly checks for errors so `ExprError()` seems like the right error reporting mechanism here as it's part of this method's interface and randomly picked caller's code in `SemaOverload.cpp` seems to handle such case. Re: tests Since you are using Sema from your code I assume you should be able to create a regression test (with a suitable code input) as a minimal reproducer. You might take a look how is `Sema` set up in unittests. Repository: rC Clang https://reviews.llvm.org/D54047 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54693: [clangd] Store source file hash in IndexFile{In,Out}
jkorous added a comment.
Hi Kadir, I have one small nit otherwise LGTM.
Comment at: clangd/index/Serialization.cpp:368
+Reader Hash(Chunks.lookup("hash"));
+llvm::StringRef Digest = Hash.consume(20);
+Result.Digest.emplace();
Nit: Maybe we could use `Result.Digest.size()` and avoid having `20` hardcoded
here?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous created this revision.
jkorous added reviewers: sammccall, arphaman, benlangmuir.
Herald added subscribers: cfe-commits, kadircet, dexonsmith, MaskRay, ioeric,
ilya-biryukov.
Hi,
I implemented `textDocument/cursorInfo` method based on consensus in
https://reviews.llvm.org/D54529.
I'd like to ask for early feedback - what's still missing is relevant client
capability.
Couple things that I'd love to hear opinions about are:
- conditional return in `getCursorInfo` - Should we return for example data
with empty `USR`?
- `containerName` of local variables - It's currently empty even if semantic
parent has a name (say it's a function). (Please search for local.cpp in the
test.) Is that what we want?
- For now I used `getSymbolAtPosition()` as it's simpler and fits the context
better. However I assume I could use this optimization from
`tooling::getNamedDeclAt()` (in a separate patch):
https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/USRFinder.cpp#L82
- One thing I am wondering about is whether we could use (and whether it's a
significant improvement) some early return in `indexTopLevelDecls` (using
`DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it
correct to assume that at a given `Location` there can be maximum of one
declaration and one definition?
P. S. Alex and Ben have thanksgiving break now so they'll probably add any
feedback next week.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
ClangdServer.cpp
ClangdServer.h
Protocol.cpp
Protocol.h
XRefs.cpp
XRefs.h
clangd/cursor-info.test
Index: clangd/cursor-info.test
===
--- /dev/null
+++ clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/cursorInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: XRefs.h
===
--- XRefs.h
+++ XRefs.h
@@ -38,6 +38,9 @@
std::vector findReferences(ParsedAST &AST, Position Pos,
const Symbo
