[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations
rsmith added a comment. In https://reviews.llvm.org/D53787#1279913, @phosek wrote: > In https://reviews.llvm.org/D53787#1279899, @rsmith wrote: > > > Replacing the global new and delete is supposed to be a whole-program > > operation (you only get one global allocator). Otherwise you couldn't > > allocate memory in one DSO and deallocate it in another. (And nor could > > inline functions etc.) > > > > If you really want to do this weird thing, and you understand what you're > > getting yourself into, I don't see a problem with having a dedicated flag > > for it, but don't break all existing users of -fvisibility=. > > > That sounds reasonable, do you have any suggestion for the name of such flag? `-fglobal-new-delete-visibility=` is the first thing that springs to mind, but maybe there's something better? Repository: rC Clang https://reviews.llvm.org/D53787 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345573 - [OPENMP] Fix for "error: unused variable 'CED'"
Author: bjope
Date: Tue Oct 30 01:49:26 2018
New Revision: 345573
URL: http://llvm.org/viewvc/llvm-project?rev=345573&view=rev
Log:
[OPENMP] Fix for "error: unused variable 'CED'"
Quick fix to make code compile with -Werror,-Wunused-variable.
Modified:
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
Modified: cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp?rev=345573&r1=345572&r2=345573&view=diff
==
--- cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp Tue Oct 30 01:49:26 2018
@@ -5018,7 +5018,7 @@ void CodeGenFunction::EmitSimpleOMPExecu
LoopGlobals.addPrivate(
VD, [&GlobLVal]() { return GlobLVal.getAddress(); });
}
- if (const auto *CED = dyn_cast(VD)) {
+ if (isa(VD)) {
// Emit only those that were not explicitly referenced in clauses.
if (!CGF.LocalDeclMap.count(VD))
CGF.EmitVarDecl(*VD);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl
takuto.ikuta added a comment.
In https://reviews.llvm.org/D51340#1278799, @hans wrote:
> I've been thinking more about your example with static locals in lambda and
> how this works with regular dllexport.
>
> It got me thinking more about the problem of exposing inline functions from a
> library. For example:
>
> `lib.h`:
>
> #ifndef LIB_H
> #define LIB_H
>
> int foo();
>
> struct __declspec(dllimport) S {
> int bar() { return foo(); }
> };
>
> #endif
>
>
>
> `lib.cc`:
>
> #include "lib.h"
>
> int foo() { return 123; }
>
>
> `main.cc`:
>
> #include "lib.h"
>
> int main() {
> S s;
> return s.bar();
> }
>
>
> Here, Clang will not emit the body of `S::bar()`, because it references the
> non-dllimport function `foo()`, and trying to referencing `foo()` outside the
> library would result in a link error. This is what the
> `DLLImportFunctionVisitor` is used for. For the same reason, MSVC will also
> not inline dllimport functions.
>
> Now, with `/Zc:dllexportInlines-`, we no longer mark `S::bar()` dllimport,
> and so we do emit it, causing that link error. The same problem happens with
> `-fvisibility-inlines-hidden`: substitute the `declspec` above for
> `__attribute__((visibility("default")))` above and try it:
>
> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o
> lib.so lib.cc
> $ g++ main.cc lib.so
> /tmp/cc557J3i.o: In function `S::bar()':
> main.cc:(.text._ZN1S3barEv[_ZN1S3barEv]+0xd): undefined reference to `foo()'
>
>
> So this is a known issue with `-fvisibility-inlines-hidden`. This doesn't
> come up often in practice, but when it does the developer needs to deal with
> it.
Yeah, that is the reason of few chromium code changes.
https://chromium-review.googlesource.com/c/chromium/src/+/1212379
> However, the static local problem is much scarier, because that leads to
> run-time bugs:
>
> `lib.h`:
>
> #ifndef LIB_H
> #define LIB_H
>
> inline int foo() { static int x = 0; return x++; }
>
> struct __attribute__((visibility("default"))) S {
> int bar() { return foo(); }
> int baz();
> };
>
> #endif
>
>
> `lib.cc`:
>
> #include "lib.h"
>
> int S::baz() { return foo(); }
>
>
> `main.cc`:
>
> #include
> #include "lib.h"
>
> int main() {
> S s;
> printf("s.bar(): %d\n", s.bar());
> printf("s.baz(): %d\n", s.baz());
> return 0;
> }
>
>
> If we build the program normally, we get the expected output:
>
> $ g++ lib.cc main.cc && ./a.out
> s.bar(): 0
> s.baz(): 1
>
>
> but building as a shared library shows the problem:
>
> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o
> lib.so lib.cc
> $ g++ main.cc lib.so
> $ LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
> s.bar(): 0
> s.baz(): 0
>
>
> Oops.
>
> This does show that it's a pre-existing problem with the model of not
> exporting inline functions though. I don't think we need to solve this
> specifically for Windows, I think we should match what
> `-fvisibility-inlines-hidden` is doing.
Currently this CL doesn't take care of inline function that is not member of a
class.
`lib.h`:
#ifndef LIB_H
#define LIB_H
struct __attribute__((visibility("default"))) S {
int bar() {
static int x = 0; return x++;
}
int baz();
};
#endif
`lib.cc`:
#include "lib.h"
int S::baz() {
return bar();
}
Then, static local in inline function is treated correctly.
$ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o lib.so
lib.cc
$ g++ main.cc lib.so
$ LD_LIBRARY_PATH=$PWD:$LD_LIBRARY_PATH ./a.out
s.bar(): 0
s.baz(): 1
This is the same behavior with `/Zc:dllexportInlines-`.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+if (ClassExported) {
hans wrote:
> takuto.ikuta wrote:
> > takuto.ikuta wrote:
> > > takuto.ikuta wrote:
> > > > hans wrote:
> > > > > takuto.ikuta wrote:
> > > > > > hans wrote:
> > > > > > > I still don't understand why we need these checks for template
> > > > > > > instantiations. Why does it matter whether the functions get
> > > > > > > inlined or not?
> > > > > > When function of dllimported class is not inlined, such funtion
> > > > > > needs to be dllexported from implementation library.
> > > > > >
> > > > > > c.h
> > > > > > ```
> > > > > > template
> > > > > > class EXPORT C {
> > > > > > public:
> > > > > > void f() {}
> > > > > > };
> > > > > > ```
> > > > > >
> > > > > > cuser.cc
> > > > > > ```
> > > > > > #include "c.h"
> > > > > >
> > > > > > void cuser() {
> > > > > > C c;
> > > > > > c.f(); // This function may not be inlined when EXPORT is
> > > > > > __declspec(dllimport), so link may fail.
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > When cuser.cc and c.h are built to diff
[PATCH] D53654: [clang] Improve ctor initializer completions.
kadircet added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:5135 + }; + auto AddDefaultCtorInit = [&](const char *TypedName, +const char *TypeName, ZaMaZaN4iK wrote: > Is it good idea to capture ALL by reference? Probably will be better to > capture only required things by reference That's the case within the rest of the code, so I did that to be consistent, and apart from that I don't think this brings any disadvantages. Repository: rC Clang https://reviews.llvm.org/D53654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47819: [test] Support using libtirpc on Linux
mgorny updated this revision to Diff 171647.
mgorny added a comment.
Rebased.
https://reviews.llvm.org/D47819
Files:
cmake/base-config-ix.cmake
lib/sanitizer_common/CMakeLists.txt
test/msan/lit.cfg
test/msan/lit.site.cfg.in
test/tsan/lit.cfg
test/tsan/lit.site.cfg.in
Index: test/tsan/lit.site.cfg.in
===
--- test/tsan/lit.site.cfg.in
+++ test/tsan/lit.site.cfg.in
@@ -6,6 +6,7 @@
config.apple_platform = "@TSAN_TEST_APPLE_PLATFORM@"
config.target_cflags = "@TSAN_TEST_TARGET_CFLAGS@"
config.target_arch = "@TSAN_TEST_TARGET_ARCH@"
+config.rpc_cflags = "@TIRPC_CFLAGS@"
# Load common config for all compiler-rt lit tests.
lit_config.load_config(config,
"@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
Index: test/tsan/lit.cfg
===
--- test/tsan/lit.cfg
+++ test/tsan/lit.cfg
@@ -49,7 +49,8 @@
[config.target_cflags] +
config.debug_info_flags +
extra_cflags +
- ["-I%s" % tsan_incdir])
+ ["-I%s" % tsan_incdir] +
+ [config.rpc_cflags])
clang_tsan_cxxflags = config.cxx_mode_flags + clang_tsan_cflags +
["-std=c++11"] + ["-I%s" % tsan_incdir]
# Add additional flags if we're using instrumented libc++.
# Instrumented libcxx currently not supported on Darwin.
Index: test/msan/lit.site.cfg.in
===
--- test/msan/lit.site.cfg.in
+++ test/msan/lit.site.cfg.in
@@ -6,6 +6,7 @@
config.target_arch = "@MSAN_TEST_TARGET_ARCH@"
config.use_lld = @MSAN_TEST_USE_LLD@
config.use_thinlto = @MSAN_TEST_USE_THINLTO@
+config.rpc_cflags = "@TIRPC_CFLAGS@"
# Load common config for all compiler-rt lit tests.
lit_config.load_config(config,
"@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
Index: test/msan/lit.cfg
===
--- test/msan/lit.cfg
+++ test/msan/lit.cfg
@@ -14,7 +14,8 @@
"-fno-omit-frame-pointer",
"-fno-optimize-sibling-calls"] +
[config.target_cflags] +
- config.debug_info_flags)
+ config.debug_info_flags +
+ [config.rpc_cflags])
# Some Msan tests leverage backtrace() which requires libexecinfo on FreeBSD.
if config.host_os == 'FreeBSD':
clang_msan_cflags += ["-lexecinfo", "-fPIC"]
Index: lib/sanitizer_common/CMakeLists.txt
===
--- lib/sanitizer_common/CMakeLists.txt
+++ lib/sanitizer_common/CMakeLists.txt
@@ -191,8 +191,6 @@
set(SANITIZER_COMMON_DEFINITIONS)
-include(FindPkgConfig)
-pkg_check_modules(TIRPC libtirpc)
if (TIRPC_FOUND)
include_directories(${TIRPC_INCLUDE_DIRS})
set(CMAKE_REQUIRED_INCLUDES ${TIRPC_INCLUDE_DIRS})
Index: cmake/base-config-ix.cmake
===
--- cmake/base-config-ix.cmake
+++ cmake/base-config-ix.cmake
@@ -8,6 +8,10 @@
check_include_file(unwind.h HAVE_UNWIND_H)
+# used in sanitizer_common and tests
+include(FindPkgConfig)
+pkg_check_modules(TIRPC libtirpc)
+
# Top level target used to build all compiler-rt libraries.
add_custom_target(compiler-rt ALL)
add_custom_target(install-compiler-rt)
Index: test/tsan/lit.site.cfg.in
===
--- test/tsan/lit.site.cfg.in
+++ test/tsan/lit.site.cfg.in
@@ -6,6 +6,7 @@
config.apple_platform = "@TSAN_TEST_APPLE_PLATFORM@"
config.target_cflags = "@TSAN_TEST_TARGET_CFLAGS@"
config.target_arch = "@TSAN_TEST_TARGET_ARCH@"
+config.rpc_cflags = "@TIRPC_CFLAGS@"
# Load common config for all compiler-rt lit tests.
lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
Index: test/tsan/lit.cfg
===
--- test/tsan/lit.cfg
+++ test/tsan/lit.cfg
@@ -49,7 +49,8 @@
[config.target_cflags] +
config.debug_info_flags +
extra_cflags +
- ["-I%s" % tsan_incdir])
+ ["-I%s" % tsan_incdir] +
+ [config.rpc_cflags])
clang_tsan_cxxflags = config.cxx_mode_flags + clang_tsan_cflags + ["-std=c++11"] + ["-I%s" % tsan_incdir]
# Add additional flags if we're using instrumented libc++.
# Instrumented libcxx currently not supported on Darwin.
Index: test/msan/lit.site.cfg.in
===
--- test/msan/lit.site.cfg.in
+++ test/msan/lit.site.cfg.in
@@ -6,6 +6,7 @@
config.target_arch = "@MSAN_TEST_TARGET_ARCH@"
config.use_lld = @MSAN_TEST_USE_LLD@
config.use_thinlto = @MSAN_TEST_USE_THINLTO@
+config.rpc_cflags = "@TIRPC_CFLAGS@"
# Load common config for all compiler
[PATCH] D47817: [sanitizer_common] Fix using libtirpc on Linux
mgorny updated this revision to Diff 171646.
mgorny added a comment.
Rebased.
https://reviews.llvm.org/D47817
Files:
lib/sanitizer_common/CMakeLists.txt
lib/sanitizer_common/sanitizer_platform_limits_posix.cc
Index: lib/sanitizer_common/sanitizer_platform_limits_posix.cc
===
--- lib/sanitizer_common/sanitizer_platform_limits_posix.cc
+++ lib/sanitizer_common/sanitizer_platform_limits_posix.cc
@@ -146,8 +146,6 @@
#include
#if HAVE_RPC_XDR_H
# include
-#elif HAVE_TIRPC_RPC_XDR_H
-# include
#endif
#include
#include
@@ -1243,7 +1241,7 @@
CHECK_SIZE_AND_OFFSET(group, gr_gid);
CHECK_SIZE_AND_OFFSET(group, gr_mem);
-#if HAVE_RPC_XDR_H || HAVE_TIRPC_RPC_XDR_H
+#if HAVE_RPC_XDR_H
CHECK_TYPE_SIZE(XDR);
CHECK_SIZE_AND_OFFSET(XDR, x_op);
CHECK_SIZE_AND_OFFSET(XDR, x_ops);
Index: lib/sanitizer_common/CMakeLists.txt
===
--- lib/sanitizer_common/CMakeLists.txt
+++ lib/sanitizer_common/CMakeLists.txt
@@ -191,9 +191,18 @@
set(SANITIZER_COMMON_DEFINITIONS)
+include(FindPkgConfig)
+pkg_check_modules(TIRPC libtirpc)
+if (TIRPC_FOUND)
+ include_directories(${TIRPC_INCLUDE_DIRS})
+ set(CMAKE_REQUIRED_INCLUDES ${TIRPC_INCLUDE_DIRS})
+endif()
+
include(CheckIncludeFile)
+cmake_push_check_state()
+string(REPLACE "-nodefaultlibs" "" CMAKE_REQUIRED_FLAGS
${CMAKE_REQUIRED_FLAGS})
append_have_file_definition(rpc/xdr.h HAVE_RPC_XDR_H
SANITIZER_COMMON_DEFINITIONS)
-append_have_file_definition(tirpc/rpc/xdr.h HAVE_TIRPC_RPC_XDR_H
SANITIZER_COMMON_DEFINITIONS)
+cmake_pop_check_state()
set(SANITIZER_CFLAGS ${SANITIZER_COMMON_CFLAGS})
append_rtti_flag(OFF SANITIZER_CFLAGS)
Index: lib/sanitizer_common/sanitizer_platform_limits_posix.cc
===
--- lib/sanitizer_common/sanitizer_platform_limits_posix.cc
+++ lib/sanitizer_common/sanitizer_platform_limits_posix.cc
@@ -146,8 +146,6 @@
#include
#if HAVE_RPC_XDR_H
# include
-#elif HAVE_TIRPC_RPC_XDR_H
-# include
#endif
#include
#include
@@ -1243,7 +1241,7 @@
CHECK_SIZE_AND_OFFSET(group, gr_gid);
CHECK_SIZE_AND_OFFSET(group, gr_mem);
-#if HAVE_RPC_XDR_H || HAVE_TIRPC_RPC_XDR_H
+#if HAVE_RPC_XDR_H
CHECK_TYPE_SIZE(XDR);
CHECK_SIZE_AND_OFFSET(XDR, x_op);
CHECK_SIZE_AND_OFFSET(XDR, x_ops);
Index: lib/sanitizer_common/CMakeLists.txt
===
--- lib/sanitizer_common/CMakeLists.txt
+++ lib/sanitizer_common/CMakeLists.txt
@@ -191,9 +191,18 @@
set(SANITIZER_COMMON_DEFINITIONS)
+include(FindPkgConfig)
+pkg_check_modules(TIRPC libtirpc)
+if (TIRPC_FOUND)
+ include_directories(${TIRPC_INCLUDE_DIRS})
+ set(CMAKE_REQUIRED_INCLUDES ${TIRPC_INCLUDE_DIRS})
+endif()
+
include(CheckIncludeFile)
+cmake_push_check_state()
+string(REPLACE "-nodefaultlibs" "" CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
append_have_file_definition(rpc/xdr.h HAVE_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
-append_have_file_definition(tirpc/rpc/xdr.h HAVE_TIRPC_RPC_XDR_H SANITIZER_COMMON_DEFINITIONS)
+cmake_pop_check_state()
set(SANITIZER_CFLAGS ${SANITIZER_COMMON_CFLAGS})
append_rtti_flag(OFF SANITIZER_CFLAGS)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53808: [clangd] Don't collect refs from non-canonical headers.
sammccall added a comment.
It's not obvious whether/why this is the right thing to do.
One set of reasoning that occurs to me:
- headers without guards produce code in some context-sensitive way, they only
exist in the context of their including file
- for the symbols they produce, we have ways to reconcile across contexts - by
symbol ID
- but for refs, there's no such context of identity
- therefore it's not clear whether a ref in an unguarded header should be
tracked once, once per include, or not at all. Not at all is defensible.
This falls down a bit though when we see that in practice we'll often have refs
to the same symbol, from the same location, with the same role - this is a
reasonable way to reconcile.
If this is just a way to trim down index size, we should consider codebases
other than LLVM, as it seems pretty likely the use of generated files there is
atypical.
Comment at: clangd/index/SymbolCollector.cpp:479
const auto &SM = ASTCtx->getSourceManager();
+ llvm::DenseSet CanonicalHeaders;
+ for (auto &IT : PP->macros()) {
High-level comment - what are you doing here? Why?
Comment at: clangd/index/SymbolCollector.cpp:479
const auto &SM = ASTCtx->getSourceManager();
+ llvm::DenseSet CanonicalHeaders;
+ for (auto &IT : PP->macros()) {
sammccall wrote:
> High-level comment - what are you doing here? Why?
Use of "canonical" here and elsewhere isn't obvious to me - does this name come
from somewhere?
"Modular" is how I would describe these headers - it's imprecise, but you could
define it somewhere.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53808
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
rjmccall added a comment. That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make their destructors protected in order to prevent types from being constructed except as base sub-objects, which is still valid here. But at the same time, at a low level, we are directly accessing a protected destructor from a context that is not code actually defined in a subclass. Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52949: [Diagnostics] Implement -Wsizeof-pointer-div
xbolva00 added a comment. Ping @MTC or @rsmith https://reviews.llvm.org/D52949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl
hans added a comment.
>> $ g++ -fvisibility=hidden -fvisibility-inlines-hidden -fpic -shared -o
>> lib.so lib.cc
>> $ g++ main.cc lib.so
>> /tmp/cc557J3i.o: In function `S::bar()':
>> main.cc:(.text._ZN1S3barEv[_ZN1S3barEv]+0xd): undefined reference to
>> `foo()'
>>
>>
>> So this is a known issue with `-fvisibility-inlines-hidden`. This doesn't
>> come up often in practice, but when it does the developer needs to deal with
>> it.
>
> Yeah, that is the reason of few chromium code changes.
> https://chromium-review.googlesource.com/c/chromium/src/+/1212379
Ah, thanks! I hadn't seen what the fixes would look like.
>> However, the static local problem is much scarier, because that leads to
>> run-time bugs:
>
> Currently this CL doesn't take care of inline function that is not member of
> a class.
>
> `lib.h`:
>
> #ifndef LIB_H
> #define LIB_H
>
> struct __attribute__((visibility("default"))) S {
> int bar() {
> static int x = 0; return x++;
> }
> int baz();
> };
>
> #endif
>
>
> `lib.cc`:
>
> #include "lib.h"
>
> int S::baz() {
> return bar();
> }
>
>
> Then, static local in inline function is treated correctly.
I think it's possible to get the same problem with member functions, but that
doesn't matter, it's an existing problem so we don't need to solve it, just be
aware.
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5719
+ TSK != TSK_ExplicitInstantiationDeclaration &&
+ TSK != TSK_ExplicitInstantiationDefinition) {
+if (ClassExported) {
takuto.ikuta wrote:
> hans wrote:
> > takuto.ikuta wrote:
> > > takuto.ikuta wrote:
> > > > takuto.ikuta wrote:
> > > > > hans wrote:
> > > > > > takuto.ikuta wrote:
> > > > > > > hans wrote:
> > > > > > > > I still don't understand why we need these checks for template
> > > > > > > > instantiations. Why does it matter whether the functions get
> > > > > > > > inlined or not?
> > > > > > > When function of dllimported class is not inlined, such funtion
> > > > > > > needs to be dllexported from implementation library.
> > > > > > >
> > > > > > > c.h
> > > > > > > ```
> > > > > > > template
> > > > > > > class EXPORT C {
> > > > > > > public:
> > > > > > > void f() {}
> > > > > > > };
> > > > > > > ```
> > > > > > >
> > > > > > > cuser.cc
> > > > > > > ```
> > > > > > > #include "c.h"
> > > > > > >
> > > > > > > void cuser() {
> > > > > > > C c;
> > > > > > > c.f(); // This function may not be inlined when EXPORT is
> > > > > > > __declspec(dllimport), so link may fail.
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > When cuser.cc and c.h are built to different library, cuser.cc
> > > > > > > needs to be able to see dllexported C::f() when C::f() is not
> > > > > > > inlined.
> > > > > > > This is my understanding.
> > > > > > Your example doesn't use explicit instantiation definition or
> > > > > > declaration, so doesn't apply here I think.
> > > > > >
> > > > > > As long as the dllexport and dllimport attributes matches it's
> > > > > > fine. It doesn't matter whether the function gets inlined or not,
> > > > > > the only thing that matters is that if it's marked dllimport on the
> > > > > > "consumer side", it must be dllexport when building the dll.
> > > > > Hmm, I changed the linkage for functions having
> > > > > DLLExport/ImportStaticLocal Attr.
> > > > >
> > > > I changed linkage in ASTContext so that inline function is emitted when
> > > > it is necessary when we use fno-dllexport-inlines.
> > > I revived explicit template instantiation check.
> > > Found that this is necessary.
> > >
> > > For explicit template instantiation, inheriting dll attribute from
> > > function for local static var is run before inheriting dll attribute from
> > > class for member functions. So I cannot detect local static var of inline
> > > function after class level dll attribute processing for explicit template
> > > instantiation.
> > >
> > Oh I see, it's a static local problem..
> > Can you provide a concrete example that does not work without your check?
> > Maybe this is the right thing to do, but I would like to understand exactly
> > what the problem is.
> For the following code
> ```
> template
> class M{
> public:
>
> T Inclass() {
> static T static_x;
> ++static_x;
> return static_x;
> }
> };
>
> template class __declspec(dllexport) M;
>
> extern template class __declspec(dllimport) M;
>
> int f (){
> M mi;
> M ms;
> return mi.Inclass() + ms.Inclass();
> }
>
> ```
>
> llvm code without instantiation check become like below. Both inline
> functions of M and M is not dllimported/exported.
> ```
> $"?Inclass@?$M@H@@QEAAHXZ" = comdat any
>
> $"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = comdat any
>
> @"?static_x@?2??Inclass@?$M@H@@QEAAHXZ@4HA" = linkonce_odr dso_local global
> i32 0, comdat, align 4
>
> ; Function Attrs: noinline nounwind optnone
> define weak_
[PATCH] D53651: [clangd] Use thread pool for background indexing.
sammccall added inline comments.
Comment at: clangd/Threading.cpp:102
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
don't you also need to actually include it?
Comment at: clangd/Threading.h:120
+
+enum ThreadPriority {
+ LOW = 0,
nit: enum class since this is at clangd scope
Comment at: clangd/Threading.h:121
+enum ThreadPriority {
+ LOW = 0,
+ NORMAL = 1,
nit: llvm tends to spell enum values like `Low`, `Normal`
Comment at: clangd/Threading.h:124
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
This deserves a comment - in particular that it may be a no-op.
Comment at: clangd/index/Background.cpp:35
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while(ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
nit: clang-format
Comment at: clangd/index/Background.cpp:37
+ThreadPool.emplace_back([this] { run(); });
+setThreadPriority(ThreadPool.back(), ThreadPriority::LOW);
+ }
we discussed having different priority tasks, which would require priorities to
be set up differently (to avoid workers never waking up to consume high
priority tasks).
OK to leave this for now, but deserves a comment.
Comment at: clangd/index/Background.h:80
+ // Must be last, spawned thread reads instance vars.
+ llvm::SmallVector ThreadPool;
};
kadircet wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Why not `std::vector`? Memory allocs won't ever be a bottleneck here.
> > ilya was saying nice things about `llvm::ThreadPool` recently - worth a
> > look?
> going for llvm::ThreadPool
Any problems with llvm::ThreadPool?
Comment at: clangd/index/Background.h:19
#include "llvm/Support/SHA1.h"
+#include "TUScheduler.h"
#include
depending on TUScheduler doesn't make sense here. Move the function to
Threading.h instead?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345576 - Remove trailing space from clang/Basic/LangOptions.h
Author: tikuta
Date: Tue Oct 30 02:42:49 2018
New Revision: 345576
URL: http://llvm.org/viewvc/llvm-project?rev=345576&view=rev
Log:
Remove trailing space from clang/Basic/LangOptions.h
Modified:
cfe/trunk/include/clang/Basic/LangOptions.h
Modified: cfe/trunk/include/clang/Basic/LangOptions.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.h?rev=345576&r1=345575&r2=345576&view=diff
==
--- cfe/trunk/include/clang/Basic/LangOptions.h (original)
+++ cfe/trunk/include/clang/Basic/LangOptions.h Tue Oct 30 02:42:49 2018
@@ -298,7 +298,7 @@ public:
/// Floating point control options
class FPOptions {
public:
- FPOptions() : fp_contract(LangOptions::FPC_Off),
+ FPOptions() : fp_contract(LangOptions::FPC_Off),
fenv_access(LangOptions::FEA_Off) {}
// Used for serializing.
@@ -344,7 +344,7 @@ public:
unsigned getInt() const { return fp_contract | (fenv_access << 2); }
private:
- /// Adjust BinaryOperator::FPFeatures to match the total bit-field size
+ /// Adjust BinaryOperator::FPFeatures to match the total bit-field size
/// of these two.
unsigned fp_contract : 2;
unsigned fenv_access : 1;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53651: [clangd] Use thread pool for background indexing.
ilya-biryukov added inline comments.
Comment at: clangd/Threading.cpp:101
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
Maybe put this helper into `llvm/Support/Threading.h`?
Comment at: clangd/Threading.h:120
+
+enum ThreadPriority {
+ LOW = 0,
sammccall wrote:
> nit: enum class since this is at clangd scope
Maybe use `enum class`?
Comment at: clangd/index/Background.h:19
#include "llvm/Support/SHA1.h"
+#include "TUScheduler.h"
#include
sammccall wrote:
> depending on TUScheduler doesn't make sense here. Move the function to
> Threading.h instead?
Maybe move the `getDefaultAsyncThreadsCount` into `Threading.h`?
Having a dependency on TUScheduler in Background indexing is a bit misleading,
since we're not actually sharing any threading infrastructure there.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53654: [clang] Improve ctor initializer completions.
ilya-biryukov added inline comments.
Comment at: lib/Sema/SemaCodeComplete.cpp:813
+static void AddTemplateParameterChunks(ASTContext &Context,
+ const PrintingPolicy &Policy,
We don't seem to need this fwd-decl anymore. Remove it?
Comment at: lib/Sema/SemaCodeComplete.cpp:821
+
+DeclContext::lookup_result getConstructorResults(ASTContext &Context,
+ const CXXRecordDecl *Record,
There's a function that does the same thing and also does some extra lifting to
make sure implicit members are properly generated -
`Sema::LookupConstructors`. Maybe use it instead?
Comment at: lib/Sema/SemaCodeComplete.cpp:831
+ // template args and one parameter with type's name.
+ /*
+ if (Ctors.begin() == Ctors.end()) {
Remove this code instead of commenting it out?
Comment at: lib/Sema/SemaCodeComplete.cpp:882
+ auto Ctors = getConstructorResults(SemaRef.Context, Record, R);
+ for (DeclContext::lookup_iterator I = Ctors.begin(), E = Ctors.end(); I != E;
+ ++I) {
Maybe use range-baseed for loop?
Comment at: lib/Sema/SemaCodeComplete.cpp:5123
+
+ auto generateCCS = [&](const NamedDecl *ND, const char *Name) {
+Builder.AddTypedTextChunk(Name);
NIT: naming, use UpperCamelCase
Comment at: lib/Sema/SemaCodeComplete.cpp:5135
+ };
+ auto AddDefaultCtorInit = [&](const char *TypedName,
+const char *TypeName,
kadircet wrote:
> ZaMaZaN4iK wrote:
> > Is it good idea to capture ALL by reference? Probably will be better to
> > capture only required things by reference
> That's the case within the rest of the code, so I did that to be consistent,
> and apart from that I don't think this brings any disadvantages.
`TypedName` and `TypeName` are too similar. Maybe pick names that are easier to
distinguish?
`Name` and `Type` seem to be a good fit
Comment at: lib/Sema/SemaCodeComplete.cpp:5136
+ auto AddDefaultCtorInit = [&](const char *TypedName,
+const char *TypeName,
+const NamedDecl* ND) {
Maybe use StringRef?
Comment at: lib/Sema/SemaCodeComplete.cpp:5151
+ auto AddCtorsWithName = [&](const CXXRecordDecl *RD,
+ const CodeCompletionResult R, const char *Name) {
+auto Ctors = getConstructorResults(Context, RD, R);
`CodeCompletionResult R` looks redundant here? Do we really need it?
Comment at: lib/Sema/SemaCodeComplete.cpp:5166
+const auto *RD = Base.getType()->getAsCXXRecordDecl();
+if (!RD)
+ return AddDefaultCtorInit(BaseName, BaseName, nullptr);
The code past that point is the same between AddBase and AddField. Maybe move
it into AddCtorsWithName?
Repository:
rC Clang
https://reviews.llvm.org/D53654
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Great stuff! Just nits (though a few of them; this is a tricky patch!).
Comment at: clangd/index/Background.cpp:122
+inline BackgroundIndex::FileDigest digest(StringRef Content) {
+ return SHA1::hash({(const uint8_t *)Content.data(), Content.size()});
static rather than inline?
Comment at: clangd/index/Background.cpp:135
+
+// Resolves URI to file paths with cache.
+StringRef BackgroundIndex::uriToPath(StringRef FileURI, StringRef HintPath) {
Might be cleanest to wrap the cache, the URISchemes&, the hint path, and this
function into a tiny class
Comment at: clangd/index/Background.cpp:160
+ const StringMap &FilesToUpdate) {
+ // Partition symbols/references into files.
+ struct File {
Maybe add a comment "eventually SymbolCollector may do this for us to avoid all
this copying"?
Comment at: clangd/index/Background.cpp:311
SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
- // FIXME: partition the symbols by file rather than TU, to avoid duplication.
- IndexedSymbols.update(AbsolutePath,
-llvm::make_unique(std::move(Symbols)),
-llvm::make_unique(std::move(Refs)));
- FileHash[AbsolutePath] = Hash;
+ update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate);
+ IndexedFileDigests[AbsolutePath] = Hash;
It looks like you never write the FilesToUpdate hashes back to the
IndexedFileDigests, so we'll always update headers?
Comment at: clangd/index/Background.h:55
+ using FileDigest = decltype(llvm::SHA1::hash({}));
+ using FileDigests = llvm::StringMap;
private.
Comment at: clangd/index/Background.h:75
+ FileDigests IndexedFileDigests; // Key is absolute file path.
+ llvm::StringMap URIToPathCache;
There's no need for this to be global to the class, it's going to cause some
hassle once there are multiple worker threads.
I think can a cache can be scoped to a single call to update() (and both it and
uriToPath() can be hidden in the cpp file.)
Comment at: clangd/index/FileIndex.cpp:123
+ case DuplicateHandling::Merge: {
+// Merge slabs into a single slab and only keep alive the merged.
+DenseMap Merged;
comment is stale
Comment at: clangd/index/FileIndex.cpp:140
+ }
+ case DuplicateHandling::PickOne: {
+for (const auto &Slab : SymbolSlabs)
since we're deduplicating above, we could deduplicate here too and remove the
deduplicating logic from Dex (MemIndex gets it by mistake). That would avoid
duplicated deduplication (!) in the Merge + Dex case, and seems a little
cleaner.
Not something for this patch, but maybe add a FIXME.
Comment at: clangd/index/SymbolCollector.cpp:212
+ auto I = FilesToIndexCache->try_emplace(FID);
+ if (!I.second)
+return I.first->second;
nit: simplify logic by inverting the if here and sharing the return?
Comment at: clangd/index/SymbolCollector.cpp:441
std::string FileURI;
- if (auto DeclLoc = getTokenLocation(MI->getDefinitionLoc(), SM, Opts,
- PP->getLangOpts(), FileURI))
+ auto DefLoc = MI->getDefinitionLoc();
+ // FIXME: use the result to filter out symbols.
why is this hoisted above shouldIndexFile? or did you mean to use DefLoc below?
Comment at: clangd/index/SymbolCollector.cpp:442
+ auto DefLoc = MI->getDefinitionLoc();
+ // FIXME: use the result to filter out symbols.
+ shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
just to check - this is basically trivial to do now but would need tests etc?
(fine to defer from this patch)
Comment at: clangd/index/SymbolCollector.h:78
+/// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
+std::function FileFilter = nullptr;
};
I think we should explicitly test this in SymbolCollectorTests - I think it
should be straightforward?
Comment at: unittests/clangd/BackgroundIndexTests.cpp:28
// a.h yields different symbols when included by A.cc vs B.cc.
// Currently we store symbols for each TU, so we get both.
+ FS.Files[testPath("root/A.h")] = R"(
stale comment?
Comment at: unittests/clangd/BackgroundIndexTests.cpp:29
// Currently we store symbols for each TU, so we get both.
- FS.Files[testPath("root/A.h")] = "void a_h(); void NAME(){}";
- FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
- FS.Files[testPath("root/B.cc")] = "#define NAME bar\n#include \"A.h\"";
- BackgroundIndex Idx(Context::empty(), "", FS);
+ FS.Fil
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet updated this revision to Diff 171655.
kadircet marked 14 inline comments as done.
kadircet added a comment.
- Address comments && Use ThreadPool
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/ThreadPool.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,16 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
- void run(); // Main loop executed by Thread. Runs tasks from Queue.
+ using Task = std::function;
+ // Wraps a task with necessary tracking information.
+ Task wrapTask(Task T);
void enqueueLocked(tooling::CompileCommand Cmd);
- std::mutex QueueMu;
- unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
- std::condition_variable QueueCV;
+ std::mutex QueuedTaskMu;
+ unsigned NumQueuedTasks = 0; // Only idle when queue is empty *and* no tasks.
+ std::condition_variable QueuedTaskCV;
bool ShouldStop = false;
- std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ // Must be last, spawned thread reads instance vars.
+ llvm::ThreadPool Pool;
};
} // namespace clangd
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,62 +26,73 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes), Pool(ThreadPoolSize) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ // ThreadPool already waits for all running threads to finish on destruction.
+ // So, we do not need to wait explicitly for that.
}
void BackgroundIndex::stop() {
{
-std::lock_guard Lock(QueueMu);
+std::lock_guard Lock(QueuedTaskMu);
ShouldStop = true;
}
- QueueCV.notify_all();
+ QueuedTaskCV.notify_all();
}
-void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
- while (true) {
-Optional Task;
+BackgroundIndex::Task BackgroundIndex::wrapTask(Task T) {
+ // Set priority to low, since background indexing is a long running task do
+ // not eat up cpu when there are any other high priority threads.
+ setThreadPriority(ThreadPriority::Low);
+
+ auto Wrapped = [this](Task T) {
+WithContext Background(BackgroundContext.clone());
{
- std::unique_lock Lock(QueueMu);
- QueueCV.wait(Lock, [&] { return ShouldStop || !Queue.empty(); });
- if (ShouldStop) {
-Queue.clear();
-QueueCV.notify_all();
+ std::unique_lock Lock(QueuedTaskMu);
+ if (ShouldStop)
return;
- }
- ++NumActiveTasks;
- Task = std::move(Queue.front());
- Queue.pop_front();
}
-(*Task)();
+T();
{
- std::unique_lock Lock(QueueMu);
- assert(NumActiveTasks > 0 && "before decrementing");
- --NumActiveTasks;
+ std::unique_lock Lock(QueuedTaskMu);
+ assert(NumQueuedTasks > 0 && "before decrementing");
+ --NumQueuedTasks;
}
-QueueCV.notify_all();
- }
+QueuedTaskCV.notify_all();
+ };
+ return Bind(Wrapped, std::move(T));
}
void BackgroundIndex::blockUntilIdleForTest() {
- std::unique_lock Lock(QueueMu);
- QueueCV.wait(Lock, [&] { return Queue.empty() && NumActiveTa
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet added inline comments.
Comment at: clangd/Threading.cpp:102
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
sammccall wrote:
> don't you also need to actually include it?
Turns out it was included transitively.
Comment at: clangd/index/Background.h:19
#include "llvm/Support/SHA1.h"
+#include "TUScheduler.h"
#include
ilya-biryukov wrote:
> sammccall wrote:
> > depending on TUScheduler doesn't make sense here. Move the function to
> > Threading.h instead?
> Maybe move the `getDefaultAsyncThreadsCount` into `Threading.h`?
> Having a dependency on TUScheduler in Background indexing is a bit
> misleading, since we're not actually sharing any threading infrastructure
> there.
actually there is one in llvm with the same semantics we use(never return zero)
`llvm::hardware_concurrency`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet updated this revision to Diff 171658.
kadircet added a comment.
- Delete outdated comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/ThreadPool.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,15 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
- void run(); // Main loop executed by Thread. Runs tasks from Queue.
+ using Task = std::function;
+ // Wraps a task with necessary tracking information.
+ Task wrapTask(Task T);
void enqueueLocked(tooling::CompileCommand Cmd);
- std::mutex QueueMu;
- unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
- std::condition_variable QueueCV;
+ std::mutex QueuedTaskMu;
+ unsigned NumQueuedTasks = 0; // Only idle when queue is empty *and* no tasks.
+ std::condition_variable QueuedTaskCV;
bool ShouldStop = false;
- std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ llvm::ThreadPool Pool;
};
} // namespace clangd
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,62 +26,73 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes), Pool(ThreadPoolSize) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ // ThreadPool already waits for all running threads to finish on destruction.
+ // So, we do not need to wait explicitly for that.
}
void BackgroundIndex::stop() {
{
-std::lock_guard Lock(QueueMu);
+std::lock_guard Lock(QueuedTaskMu);
ShouldStop = true;
}
- QueueCV.notify_all();
+ QueuedTaskCV.notify_all();
}
-void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
- while (true) {
-Optional Task;
+BackgroundIndex::Task BackgroundIndex::wrapTask(Task T) {
+ // Set priority to low, since background indexing is a long running task do
+ // not eat up cpu when there are any other high priority threads.
+ setThreadPriority(ThreadPriority::Low);
+
+ auto Wrapped = [this](Task T) {
+WithContext Background(BackgroundContext.clone());
{
- std::unique_lock Lock(QueueMu);
- QueueCV.wait(Lock, [&] { return ShouldStop || !Queue.empty(); });
- if (ShouldStop) {
-Queue.clear();
-QueueCV.notify_all();
+ std::unique_lock Lock(QueuedTaskMu);
+ if (ShouldStop)
return;
- }
- ++NumActiveTasks;
- Task = std::move(Queue.front());
- Queue.pop_front();
}
-(*Task)();
+T();
{
- std::unique_lock Lock(QueueMu);
- assert(NumActiveTasks > 0 && "before decrementing");
- --NumActiveTasks;
+ std::unique_lock Lock(QueuedTaskMu);
+ assert(NumQueuedTasks > 0 && "before decrementing");
+ --NumQueuedTasks;
}
-QueueCV.notify_all();
- }
+QueuedTaskCV.notify_all();
+ };
+ return Bind(Wrapped, std::move(T));
}
void BackgroundIndex::blockUntilIdleForTest() {
- std::unique_lock Lock(QueueMu);
- QueueCV.wait(Lock, [&] { return Queue.empty() && NumActiveTasks == 0; });
+ std::unique_lock Lock(QueuedTaskMu);
+ QueuedTaskCV.wait(Lock, [&] { return NumQueuedTasks =
[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1
filcab updated this revision to Diff 171660.
filcab added a comment.
Update with name change, using rjmccall's suggestion
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
include/clang/Driver/Options.td
include/clang/Driver/SanitizerArgs.h
include/clang/Frontend/CodeGenOptions.def
lib/CodeGen/ItaniumCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -208,6 +208,24 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-WITHOUT-USE-AFTER-SCOPE
// CHECK-ASAN-WITHOUT-USE-AFTER-SCOPE: -cc1{{.*}}address-use-after-scope
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fsanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE: -cc1{{.*}}-fsanitize-address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -fsanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -fsanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH: -cc1{{.*}}-fsanitize-address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-poison-custom-array-cookie -fno-sanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-globals-dead-stripping %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-ASAN-GLOBALS
// RUN: %clang_cl --target=x86_64-windows-msvc -fsanitize=address -fsanitize-address-globals-dead-stripping -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -968,11 +968,11 @@
Args.hasArg(OPT_fsanitize_cfi_icall_generalize_pointers);
Opts.SanitizeStats = Args.hasArg(OPT_fsanitize_stats);
if (Arg *A = Args.getLastArg(
- OPT_fsanitize_address_poison_class_member_array_new_cookie,
- OPT_fno_sanitize_address_poison_class_member_array_new_cookie)) {
-Opts.SanitizeAddressPoisonClassMemberArrayNewCookie =
+ OPT_fsanitize_address_poison_custom_array_cookie,
+ OPT_fno_sanitize_address_poison_custom_array_cookie)) {
+Opts.SanitizeAddressPoisonCustomArrayCookie =
A->getOption().getID() ==
-OPT_fsanitize_address_poison_class_member_array_new_cookie;
+OPT_fsanitize_address_poison_custom_array_cookie;
}
if (Arg *A = Args.getLastArg(OPT_fsanitize_address_use_after_scope,
OPT_fno_sanitize_address_use_after_scope)) {
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -724,6 +724,11 @@
options::OPT_fsanitize_address_use_after_scope,
options::OPT_fno_sanitize_address_use_after_scope, AsanUseAfterScope);
+AsanPoisonCustomArrayCookie = Args.hasFlag(
+options::OPT_fsanitize_address_poison_custom_array_cookie,
+options::OPT_fno_sanitize
[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition
ebevhan added a comment.
In https://reviews.llvm.org/D53738#1279069, @rjmccall wrote:
> > The important difference would be that we separate the semantics of
> > performing the conversion and the operation. I understand that adding a new
> > type could be a bit more involved than baking the conversion into the
> > operator, but I really do not enjoy seeing so much implicit,
> > implementation-specific logic encapsulated in the same AST element. Anyone
> > who wants to look at BinaryOperators with fixed-point types will need to
> > know all of the details of how the finding the common type and conversion
> > is done, rather than simply "it does an addition".
>
> It's not just that adding a new type is "involved", it's that it's a bad
> solution. Those types can't actually be expressed in the language, and
> changing the type system to be able to express them is going to lead to a lot
> of pain elsewhere.
I did figure that it might be a bit of work to adapt other parts of the code to
handle the new type, but I just prefer separating the concerns and being
explicit about what work is performed. To me, the clearest way of doing that is
by handling the conversions explicitly in the AST, and separately from the
operators themselves. Also, not being able to deal in QualTypes for the common
full precision type handling means that reusing the operator handling code in
Sema won't be easy, or even possible. It would have to be computed in
CreateBuiltinBinOp, since it's not possible to forward anything but QualType
from the CheckXXXOperands functions.
If you don't think it's a good idea I'll concede, but then there's the question
of how to get the full precision semantics into the operator (if we store it
there). I guess the most straightforward path is something like:
- In CreateBuiltinBinOp, do the normal Check function to get the result type
- If the result is a fixed-point type, go into a separate code path
- Ask a method for the common FixedPointSemantics of the given LHS and RHS
- Build the correct BinaryOperator subclass.
I need to think about this to see if our downstream implementation can be
adapted to work with this design.
Compound assignments are already their own subclass, though. Unless the full
precision semantic information is simply baked into the regular BinaryOperator,
it would require two new subclasses: one for normal full precision operators
and one for compound ones? Wouldn't adding this require new hooks and code
paths in visitors, even though there's really nothing different about the
operator?
The type information that CompoundAssignOperator stores today is rather similar
to what a full precision operator would need, though it would need to store a
FixedPointSemantics instead.
---
I do have more comments on the code at the moment, but I'm holding off a bit on
the review while we iron out some of these details.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3156
+/// the type is an integer, the scale is zero.
+static void GetFixedPointAttributes(ASTContext &Ctx, QualType Ty,
+unsigned &Width, unsigned &Scale,
This function should not be necessary. Instead, add to FixedPointSemantics:
* `static FixedPointSemantics GetIntegerSemantics(unsigned Width, bool
IsSigned)` to get an FPS for a specific integer width and signedness
(width=Width, scale=0, sat=false, signed=IsSigned, padding=false)
* `FixedPointSemantics getCommonSemantics(const FixedPointSemantics &Other)` to
get an FPS for the common full precision semantic between this FPS and another
one
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3197
+ GetFixedPointAttributes(Ctx, RHSTy, RHSWidth, RHSScale, RHSSign);
+ GetFixedPointAttributes(Ctx, ResultTy, ResultWidth, ResultScale, ResultSign);
+
I think all of this (or at least something equivalent to it) should be
calculated in Sema, not in CodeGen.
If we go with the idea of storing the full precision semantics in the operator,
all the code in CodeGen should have to do is call EmitFixedPointConversion on
the LHS and RHS with the FixedPointSemantics given by the operator. Same for
converting back after the operation is performed.
Comment at: clang/lib/Sema/SemaExpr.cpp:1310
+/// For a given fixed point type, return it's signed equivalent.
+static QualType GetCorrespondingSignedFixedPointType(ASTContext &Ctx,
+ QualType Ty) {
Maybe this should be a method on ASTContext itself? It's probably useful in
other cases.
Comment at: clang/lib/Sema/SemaExpr.cpp:9219
+ else if (RHS.get()->getType()->isFixedPointType())
+compType = FixedPointConversions(RHS, LHS, CompLHSTy);
+ else
I think this 'commutativity' should be handled inside the function rather than
here.
Repository:
rC Clang
https://reviews.llvm.org/D53
[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1
filcab updated this revision to Diff 171661.
filcab added a comment.
Missed the change in some places
Repository:
rC Clang
https://reviews.llvm.org/D52615
Files:
docs/ClangCommandLineReference.rst
docs/UsersManual.rst
include/clang/Driver/Options.td
include/clang/Driver/SanitizerArgs.h
include/clang/Frontend/CodeGenOptions.def
lib/CodeGen/ItaniumCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Frontend/CompilerInvocation.cpp
test/CodeGen/address-sanitizer-and-array-cookie.cpp
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -208,6 +208,24 @@
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-WITHOUT-USE-AFTER-SCOPE
// CHECK-ASAN-WITHOUT-USE-AFTER-SCOPE: -cc1{{.*}}address-use-after-scope
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fsanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE: -cc1{{.*}}-fsanitize-address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-OFF-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -fsanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH
+// RUN: %clang_cl --target=x86_64-windows -fsanitize=address -fno-sanitize-address-poison-custom-array-cookie -fsanitize-address-poison-custom-array-cookie -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH: -cc1{{.*}}-fsanitize-address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-poison-custom-array-cookie -fno-sanitize-address-poison-custom-array-cookie %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF
+// CHECK-POISON-CUSTOM-ARRAY-NEW-COOKIE-BOTH-OFF-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE
+// CHECK-ASAN-WITHOUT-POISON-CUSTOM-ARRAY-NEW-COOKIE-NOT: -cc1{{.*}}address-poison-custom-array-cookie
+
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-globals-dead-stripping %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
// RUN: %clang -target x86_64-linux-gnu -fsanitize=address %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-NO-ASAN-GLOBALS
// RUN: %clang_cl --target=x86_64-windows-msvc -fsanitize=address -fsanitize-address-globals-dead-stripping -### -- %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-GLOBALS
Index: test/CodeGen/address-sanitizer-and-array-cookie.cpp
===
--- test/CodeGen/address-sanitizer-and-array-cookie.cpp
+++ test/CodeGen/address-sanitizer-and-array-cookie.cpp
@@ -1,6 +1,6 @@
// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - %s | FileCheck %s -check-prefix=PLAIN
// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - -fsanitize=address %s | FileCheck %s -check-prefix=ASAN
-// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - -fsanitize=address -fsanitize-address-poison-class-member-array-new-cookie %s | FileCheck %s -check-prefix=ASAN-POISON-ALL-NEW-ARRAY
+// RUN: %clang_cc1 -triple x86_64-gnu-linux -emit-llvm -o - -fsanitize=address -fsanitize-address-poison-custom-array-cookie %s | FileCheck %s -check-prefix=ASAN-POISON-ALL-NEW-ARRAY
typedef __typeof__(sizeof(0)) size_t;
namespace std {
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -968,11 +968,11 @@
Args.hasArg(OPT_fsanitize_cfi_icall_generalize_pointers);
Opts.SanitizeStats = Args.hasArg(OPT_fsanitize_stats);
if (Arg *A = Args.getLastArg(
- OPT_fsanitize_address_poison_class_member_array_new_cookie,
- OPT_fno_sanitize_address_poison_class_member_array_new_cookie)) {
-Opts.SanitizeAddressPoisonCla
[PATCH] D53830: [clang-tidy]: Abseil: new check 'abseil-upgrade-duration-conversions'
JonasToth added a comment.
Hi astrelni,
my 2cents.
Please upload the patch will full context (i believe `diff -U 9`, but check
the man pages if that doesn't work :D)
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:32
+
+ // a *= b; a /= b;
+ Finder->addMatcher(
Please add types in your examples, to make clear what actually happens.
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:118
+AST_MATCHER_P(Expr, hasSourceRange, SourceRange, Range) {
+ return Node.getSourceRange() == Range;
+}
What happens on invalid ranges? Are they considered equal, or is it forbidden
to pass them in?
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:123
+ auto HasMatchingDependentDescendant = hasDescendant(
+ expr(hasSourceRange(Node.getSourceRange()), isInstantiationDependent()));
+ return expr(anyOf(hasAncestor(
Please add a test-case where the `Node.getSourceRange()` is a macro, if not
already existing. I believe that is currently missing.
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:134
+void UpgradeDurationConversionsCheck::check(
+const ast_matchers::MatchFinder::MatchResult &Result) {
+ const auto *ArgExpr = Result.Nodes.getNodeAs("arg");
`ast_matchers::` is not necessary, as there is a `using ...` at the top of the
file.
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:158
+ *Result.Context)
+ .empty()) {
+ diag(ArgExpr->getBeginLoc(), Message);
You could ellide these braces, but I feel that this matching code be merged
into one matcher with `equalsBoundNode()` (see ASTMatcher reference).
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp:164
+
+ FixItHint Fixes[] = {FixItHint::CreateInsertion(SourceRange.getBegin(),
+ "static_cast("),
my personal preference would be removing the builtin-array and add two `<<
FixitHint` call to the `diag`.
Comment at: clang-tidy/abseil/UpgradeDurationConversionsCheck.h:19
+
+/// Finds deprecated uses of absl::Duration arithmetic operators and factories.
+///
please mark code construct with ' in comments.
Comment at: docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst:43
+behavior due to types implicitly convertible to a floating point type.
+
please remove the last empty line
Comment at: docs/clang-tidy/checks/list.rst:13
abseil-redundant-strcat-calls
+ abseil-str-cat-append
abseil-string-find-startswith
spurious changes in this file.
Comment at: test/clang-tidy/abseil-upgrade-duration-conversions.cpp:142
+
+template void templateForOpsSpecialization(T) {}
+template <>
what about non-type template parameters? Do they need consideration for the
check as well?
If i am not mistaken floats are allowed in newwer standards as well.
https://reviews.llvm.org/D53830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
JonasToth added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:30 + +.. option:: IgnoreLocationless + I think the another option name would fit better: how about `IgnoreCommandLineMacros` or the like? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1
filcab added a comment. In https://reviews.llvm.org/D52615#1278000, @rjmccall wrote: > So, three points: > > - That's not class-member-specific; you can have a placement `operator new[]` > at global scope that isn't the special `void*` placement operator and > therefore still has a cookie, and it would have just as much flexibility as a > class-member override would. So the split you're trying to describe is the > standard operators vs. custom ones. > - Anyone can provide their own definition of the standard operators; there > are some semantic restrictions on those definitions, but I'm not sure what > about those restrictions would forbid this kind of capture. > - Even with the standard implementations of the standard replaceable > operators, I'm not sure what rule would actually outlaw the client from going > from the result of `new[]` back to the cookie. > > At any rate, taking the feature as a given, the first point suggests > `-fsanitize-address-poison-custom-array-cookie` or something along those > lines. If we want a more standard-ese term than "custom", the standard > refers to its operators collectively as "library allocation functions", so > maybe "non-library". Thank you. I went with your suggestion, as I think it's close enough to be understandable. I've also changed the -cc1 argument in this patch, as it looks weird to have two different arguments (unless needed). Let me know if you'd like to keep the different names. Thank you, Filipe Repository: rC Clang https://reviews.llvm.org/D52615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers
whisperity added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465
if (ChecksEnabled[CK_InvalidatedIteratorChecker] &&
isAccessOperator(Func->getOverloadedOperator())) {
// Check for any kind of access of invalidated iterator positions
if (const auto *InstCall = dyn_cast(&Call)) {
verifyAccess(C, InstCall->getCXXThisVal());
} else {
verifyAccess(C, Call.getArgSVal(0));
This is a bit of an organisational comment (and the compiler of course
hopefully optimises this out), but could you, for the sake of code readability,
break the if-elseif-elseif-elseif chain's common check out into an outer if,
and only check for the inner parts? Thinking of something like this:
```
if (ChecksEnabled[CK_InvalidatedIteratorChecker])
{
/* yadda */
}
if (ChecksEnabled[CK_IteratorRangeChecker])
{
X* OOperator = Func->getOverloadedOperator();
if (isIncrementOperator(OOperator))
{
/* yadda */
} else if (isDecrementOperator(OOperator)) {
/* yadda */
}
}
/* etc. */
```
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1074
- auto &SymMgr = C.getSymbolManager();
- auto &SVB = C.getSValBuilder();
- auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
- const auto OldOffset = Pos->getOffset();
- const auto intValue = value.getAs();
- if (!intValue)
+ // Incremention or decremention by 0 is never bug
+ if (isZero(State, Value.castAs()))
is never a, also `.` at the end
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1568
+IteratorPosition IteratorChecker::shiftPosition(CheckerContext &C,
+OverloadedOperatorKind Op,
(Minor: I don't like the name of this function, `advancePosition` (akin to
`std::advance`) sounds cleaner to my ears.)
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1575
+ auto &SVB = C.getSValBuilder();
+ auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
+ if (const auto IntDist = Distance.getAs()) {
For the sake of clarity, I think an assert should be introduced her, lest
someone ends up shifting the position with `<=`.
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2329-2330
// Out of range means less than the begin symbol or greater or equal to the
// end symbol.
How does the introduction of `IncludeEnd` change this behaviour? What does the
parameter refer to?
Repository:
rC Clang
https://reviews.llvm.org/D53812
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53125: Detect Clear Linux and apply Clear's default linker options
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM, presuming the tests pass for you. https://reviews.llvm.org/D53125 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic
Anastasia added a comment. In https://reviews.llvm.org/D53705#1279086, @rjmccall wrote: > In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > > > Unlikely, since address spaces are provided in a different way in OpenCL > > C++ vs OpenCL C. > > > > OpenCL C provides qualifiers such as `global` as part of the language. > > OpenCL C++ provides template classes such as `cl::global` through a > > header file. > > > So OpenCL C code has to be completely rewritten if it needs to be used as > part of an OpenCL C++ program? And it doesn't really compose like a type if > it's supposed to change how a variable is stored. What a terrible little > mess they've made for themselves. Fair point. I would like to allow regular OpenCL address space qualifiers as a Clang feature at least, in case we won't be able to alter the spec. But one problem is that the `private` address space qualifier keyword conflicts with the `private` class access modifier. I guess we can only allow the address space qualifiers with the `__` prefix. So some existing code would have to be changed when ported to OpenCL C++, but it should be easier than rewriting using classes. Repository: rC Clang https://reviews.llvm.org/D53705 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49486: [cfe][CMake] Export the clang resource directory
mgorny added a comment. @philip.pfaffe, did you establish whether this is still necessary or can be abandoned? https://reviews.llvm.org/D49486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet updated this revision to Diff 171672.
kadircet added a comment.
- Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/Threading.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,15 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
+ using Task = std::function;
void run(); // Main loop executed by Thread. Runs tasks from Queue.
void enqueueLocked(tooling::CompileCommand Cmd);
std::mutex QueueMu;
unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
std::condition_variable QueueCV;
bool ShouldStop = false;
std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ std::vector ThreadPool; // FIXME: Abstract this away.
};
} // namespace clangd
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,14 +26,22 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while(ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
+setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
+ }
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ for (auto& Thread : ThreadPool)
+Thread.join();
}
void BackgroundIndex::stop() {
@@ -44,7 +53,7 @@
}
void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
+ WithContext Background(BackgroundContext.clone());
while (true) {
Optional Task;
{
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
namespace clang {
@@ -115,6 +116,13 @@
mutable std::condition_variable TasksReachedZero;
std::size_t InFlightTasks = 0;
};
+
+enum class ThreadPriority {
+ Low = 0,
+ Normal = 1,
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -1,9 +1,13 @@
#include "Threading.h"
#include "Trace.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Config/config.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Threading.h"
#include
+#ifdef HAVE_PTHREAD_H
+#include
+#endif
using namespace llvm;
namespace clang {
@@ -97,5 +101,15 @@
CV.wait_until(Lock, D.time());
}
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
+ priority.sched_priority = 0;
+ pthread_setschedparam(
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priority);
+#endif
+}
+
} // namespace clangd
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50256: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager (for == and != only)
baloghadamsoftware added a comment. Herald added subscribers: donat.nagy, Szelethus. @NoQ Any comments/concerns regarding this solution? Repository: rC Clang https://reviews.llvm.org/D50256 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49486: [cfe][CMake] Export the clang resource directory
philip.pfaffe abandoned this revision. philip.pfaffe added a comment. It looks like running clang will be good enough. I'm closing this for now! https://reviews.llvm.org/D49486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53651: [clangd] Use thread pool for background indexing.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice! Sorry about the back and forth with threadpool...
Comment at: clangd/index/Background.cpp:35
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while(ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
sammccall wrote:
> nit: clang-format
(need to clang-format again)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet updated this revision to Diff 171675.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Add comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/Threading.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,15 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
+ using Task = std::function;
void run(); // Main loop executed by Thread. Runs tasks from Queue.
void enqueueLocked(tooling::CompileCommand Cmd);
std::mutex QueueMu;
unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
std::condition_variable QueueCV;
bool ShouldStop = false;
std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ std::vector ThreadPool; // FIXME: Abstract this away.
};
} // namespace clangd
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,14 +26,26 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while(ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
+// Set priority to low, since background indexing is a long running task we
+// do not want to eat up cpu when there are any other high priority threads.
+// FIXME: In the future we might want a more general way of handling this to
+// support a tasks with various priorities.
+setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
+ }
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ for (auto& Thread : ThreadPool)
+Thread.join();
}
void BackgroundIndex::stop() {
@@ -44,7 +57,7 @@
}
void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
+ WithContext Background(BackgroundContext.clone());
while (true) {
Optional Task;
{
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
namespace clang {
@@ -115,6 +116,13 @@
mutable std::condition_variable TasksReachedZero;
std::size_t InFlightTasks = 0;
};
+
+enum class ThreadPriority {
+ Low = 0,
+ Normal = 1,
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -1,9 +1,13 @@
#include "Threading.h"
#include "Trace.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Config/config.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Threading.h"
#include
+#ifdef HAVE_PTHREAD_H
+#include
+#endif
using namespace llvm;
namespace clang {
@@ -97,5 +101,15 @@
CV.wait_until(Lock, D.time());
}
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
+ priority.sched_priority = 0;
+ pthread_setschedparam(
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priority);
+
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet added inline comments.
Comment at: clangd/Threading.cpp:101
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
ilya-biryukov wrote:
> Maybe put this helper into `llvm/Support/Threading.h`?
We talked with Sam about that one and rather decided to keep it here for a
while and put into LLVM later on when we are sure about it is the right use
case for everyone in LLVM with the right API.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r345590 - [clangd] Use thread pool for background indexing.
Author: kadircet
Date: Tue Oct 30 05:13:27 2018
New Revision: 345590
URL: http://llvm.org/viewvc/llvm-project?rev=345590&view=rev
Log:
[clangd] Use thread pool for background indexing.
Reviewers: sammccall, ioeric
Reviewed By: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, jfb, cfe-commits
Differential Revision: https://reviews.llvm.org/D53651
Modified:
clang-tools-extra/trunk/clangd/Threading.cpp
clang-tools-extra/trunk/clangd/Threading.h
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
Modified: clang-tools-extra/trunk/clangd/Threading.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.cpp?rev=345590&r1=345589&r2=345590&view=diff
==
--- clang-tools-extra/trunk/clangd/Threading.cpp (original)
+++ clang-tools-extra/trunk/clangd/Threading.cpp Tue Oct 30 05:13:27 2018
@@ -1,9 +1,13 @@
#include "Threading.h"
#include "Trace.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Config/config.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Threading.h"
#include
+#ifdef HAVE_PTHREAD_H
+#include
+#endif
using namespace llvm;
namespace clang {
@@ -97,5 +101,15 @@ void wait(std::unique_lock &
CV.wait_until(Lock, D.time());
}
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
+ priority.sched_priority = 0;
+ pthread_setschedparam(
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priority);
+#endif
+}
+
} // namespace clangd
} // namespace clang
Modified: clang-tools-extra/trunk/clangd/Threading.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Threading.h?rev=345590&r1=345589&r2=345590&view=diff
==
--- clang-tools-extra/trunk/clangd/Threading.h (original)
+++ clang-tools-extra/trunk/clangd/Threading.h Tue Oct 30 05:13:27 2018
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
namespace clang {
@@ -115,6 +116,13 @@ private:
mutable std::condition_variable TasksReachedZero;
std::size_t InFlightTasks = 0;
};
+
+enum class ThreadPriority {
+ Low = 0,
+ Normal = 1,
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
} // namespace clangd
} // namespace clang
#endif
Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=345590&r1=345589&r2=345590&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Tue Oct 30 05:13:27 2018
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,14 +26,26 @@ namespace clangd {
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while (ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
+// Set priority to low, since background indexing is a long running task we
+// do not want to eat up cpu when there are any other high priority
threads.
+// FIXME: In the future we might want a more general way of handling this
to
+// support a tasks with various priorities.
+setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
+ }
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ for (auto &Thread : ThreadPool)
+Thread.join();
}
void BackgroundIndex::stop() {
@@ -44,7 +57,7 @@ void BackgroundIndex::stop() {
}
void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
+ WithContext Background(BackgroundContext.clone());
while (true) {
Optional Task;
{
Modified: clang-tools-extra/trunk/clangd/index/Background.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=345590&r1=345589&r2=345590&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.h (original)
+
[PATCH] D53651: [clangd] Use thread pool for background indexing.
kadircet updated this revision to Diff 171676.
kadircet marked an inline comment as done.
kadircet added a comment.
- Format the code.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Background.h
===
--- clangd/index/Background.h
+++ clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/Threading.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,15 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
+ using Task = std::function;
void run(); // Main loop executed by Thread. Runs tasks from Queue.
void enqueueLocked(tooling::CompileCommand Cmd);
std::mutex QueueMu;
unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
std::condition_variable QueueCV;
bool ShouldStop = false;
std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ std::vector ThreadPool; // FIXME: Abstract this away.
};
} // namespace clangd
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,14 +26,26 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while (ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
+// Set priority to low, since background indexing is a long running task we
+// do not want to eat up cpu when there are any other high priority threads.
+// FIXME: In the future we might want a more general way of handling this to
+// support a tasks with various priorities.
+setThreadPriority(ThreadPool.back(), ThreadPriority::Low);
+ }
+}
BackgroundIndex::~BackgroundIndex() {
stop();
- Thread.join();
+ for (auto &Thread : ThreadPool)
+Thread.join();
}
void BackgroundIndex::stop() {
@@ -44,7 +57,7 @@
}
void BackgroundIndex::run() {
- WithContext Background(std::move(BackgroundContext));
+ WithContext Background(BackgroundContext.clone());
while (true) {
Optional Task;
{
Index: clangd/Threading.h
===
--- clangd/Threading.h
+++ clangd/Threading.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
namespace clang {
@@ -115,6 +116,13 @@
mutable std::condition_variable TasksReachedZero;
std::size_t InFlightTasks = 0;
};
+
+enum class ThreadPriority {
+ Low = 0,
+ Normal = 1,
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
} // namespace clangd
} // namespace clang
#endif
Index: clangd/Threading.cpp
===
--- clangd/Threading.cpp
+++ clangd/Threading.cpp
@@ -1,9 +1,13 @@
#include "Threading.h"
#include "Trace.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Config/config.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Threading.h"
#include
+#ifdef HAVE_PTHREAD_H
+#include
+#endif
using namespace llvm;
namespace clang {
@@ -97,5 +101,15 @@
CV.wait_until(Lock, D.time());
}
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
+ priority.sched_priority = 0;
+ pthread_setschedparam(
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priorit
[PATCH] D53651: [clangd] Use thread pool for background indexing.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345590: [clangd] Use thread pool for background indexing.
(authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D53651
Files:
clang-tools-extra/trunk/clangd/Threading.cpp
clang-tools-extra/trunk/clangd/Threading.h
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
Index: clang-tools-extra/trunk/clangd/Threading.h
===
--- clang-tools-extra/trunk/clangd/Threading.h
+++ clang-tools-extra/trunk/clangd/Threading.h
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
namespace clang {
@@ -115,6 +116,13 @@
mutable std::condition_variable TasksReachedZero;
std::size_t InFlightTasks = 0;
};
+
+enum class ThreadPriority {
+ Low = 0,
+ Normal = 1,
+};
+void setThreadPriority(std::thread &T, ThreadPriority Priority);
+
} // namespace clangd
} // namespace clang
#endif
Index: clang-tools-extra/trunk/clangd/Threading.cpp
===
--- clang-tools-extra/trunk/clangd/Threading.cpp
+++ clang-tools-extra/trunk/clangd/Threading.cpp
@@ -1,9 +1,13 @@
#include "Threading.h"
#include "Trace.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Config/config.h"
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Threading.h"
#include
+#ifdef HAVE_PTHREAD_H
+#include
+#endif
using namespace llvm;
namespace clang {
@@ -97,5 +101,15 @@
CV.wait_until(Lock, D.time());
}
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
+ priority.sched_priority = 0;
+ pthread_setschedparam(
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priority);
+#endif
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/index/Background.h
===
--- clang-tools-extra/trunk/clangd/index/Background.h
+++ clang-tools-extra/trunk/clangd/index/Background.h
@@ -16,6 +16,7 @@
#include "index/Index.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/SHA1.h"
+#include "llvm/Support/Threading.h"
#include
#include
#include
@@ -34,7 +35,8 @@
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, StringRef ResourceDir,
const FileSystemProvider &,
- ArrayRef URISchemes = {});
+ ArrayRef URISchemes = {},
+ size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
// Enqueue a translation unit for indexing.
@@ -66,15 +68,15 @@
llvm::StringMap FileHash; // Digest of indexed file.
// queue management
- using Task = std::function; // FIXME: use multiple worker threads.
+ using Task = std::function;
void run(); // Main loop executed by Thread. Runs tasks from Queue.
void enqueueLocked(tooling::CompileCommand Cmd);
std::mutex QueueMu;
unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
std::condition_variable QueueCV;
bool ShouldStop = false;
std::deque Queue;
- std::thread Thread; // Must be last, spawned thread reads instance vars.
+ std::vector ThreadPool; // FIXME: Abstract this away.
};
} // namespace clangd
Index: clang-tools-extra/trunk/clangd/index/Background.cpp
===
--- clang-tools-extra/trunk/clangd/index/Background.cpp
+++ clang-tools-extra/trunk/clangd/index/Background.cpp
@@ -11,6 +11,7 @@
#include "ClangdUnit.h"
#include "Compiler.h"
#include "Logger.h"
+#include "Threading.h"
#include "Trace.h"
#include "index/IndexAction.h"
#include "index/MemIndex.h"
@@ -25,14 +26,26 @@
BackgroundIndex::BackgroundIndex(Context BackgroundContext,
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
- ArrayRef URISchemes)
+ ArrayRef URISchemes,
+ size_t ThreadPoolSize)
: SwapIndex(make_unique()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes), Thread([this] { run(); }) {}
+ URISchemes(URISchemes) {
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+ while (ThreadPoolSize--) {
+ThreadPool.emplace_back([this] { run(); });
+// Set priority to low, since background indexing is a long running task we
+// do not want to eat up cpu when there are any other high priority threads.
+// FIXME: In the future we might want a more
r345591 - [CodeGen] Disable the machine verifier on a ThinLTO test
Author: thegameg Date: Tue Oct 30 05:18:33 2018 New Revision: 345591 URL: http://llvm.org/viewvc/llvm-project?rev=345591&view=rev Log: [CodeGen] Disable the machine verifier on a ThinLTO test This allows us to turn the machine verifier on by default on X86. Modified: cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll Modified: cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll?rev=345591&r1=345590&r2=345591&view=diff == --- cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll (original) +++ cfe/trunk/test/CodeGen/thinlto-distributed-cfi-devirt.ll Tue Oct 30 05:18:33 2018 @@ -6,7 +6,9 @@ ; RUN: opt -thinlto-bc -o %t.o %s +; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436. ; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \ +; RUN: -verify-machineinstrs=0 \ ; RUN: -o %t2.index \ ; RUN: -r=%t.o,test,px \ ; RUN: -r=%t.o,_ZN1A1nEi,p \ ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52730: [analyzer] ConversionChecker: handle floating point
donat.nagy updated this revision to Diff 171678.
donat.nagy added a comment.
Use APFloat to determine precision of floating point type
Additionally, fix a typo in the tests.
Repository:
rC Clang
https://reviews.llvm.org/D52730
Files:
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
test/Analysis/conversion.c
Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
U8 = S + 10;
}
+char dontwarn6(long long x) {
+ long long y = 42;
+ y += x;
+ return y == 42;
+}
+
// C library functions, handled via apiModeling.StdCLibraryFunctions
@@ -154,7 +160,7 @@
# define EOF (-1)
char reply_string[8192];
FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
int libraryFunction2() {
int c, n;
int dig;
@@ -179,3 +185,26 @@
}
}
+double floating_point(long long a, int b) {
+ if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+ } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+ }
+ return 137;
+}
+
+double floating_point2() {
+ int a = 1 << 24;
+ long long b = 1LL << 53;
+ float f = a; // no-warning
+ double d = b; // no-warning
+ return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+ double b = a; // no-warning
+ return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
// of expressions. A warning is reported when:
// * a negative value is implicitly converted to an unsigned value in an
// assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-// value of target
+// * assignment / initialization when the source value is greater than the max
+// value of the target integer type
+// * assignment / initialization when the source integer is above the range
+// where the target floating point type can represent all integers
//
// Many compilers and tools have similar checks that are based on semantic
// analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -28,6 +30,9 @@
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+#include
using namespace clang;
using namespace ento;
@@ -40,11 +45,9 @@
private:
mutable std::unique_ptr BT;
- // Is there loss of precision
bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
CheckerContext &C) const;
- // Is there loss of sign
bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
@@ -132,19 +135,55 @@
QualType SubType = Cast->IgnoreParenImpCasts()->getType();
- if (!DestType->isIntegerType() || !SubType->isIntegerType())
+ if (!DestType->isRealType() || !SubType->isIntegerType())
return false;
- if (C.getASTContext().getIntWidth(DestType) >=
- C.getASTContext().getIntWidth(SubType))
+ const bool isInteger = DestType->isIntegerType();
+
+ const auto &AC = C.getASTContext();
+
+ // We will find the largest RepresentsUntilExp value such that the DestType
+ // can exactly represent all nonnegative integers below 2^RepresentsUntilExp.
+ unsigned RepresentsUntilExp;
+
+ if (isInteger) {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+ // This is just casting a number to bool, probably not a bug.
+ return false;
+}
+if (DestType->isSignedIntegerType())
+ RepresentsUntilExp--;
+ } else {
+unsigned FloatingSize = AC.getTypeSize(DestType);
+// getAllOneValues returns an APFloat with semantics corresponding to the
+// bit size given as the first argument; this is the only function in
+// APFloat.h that maps bit width to semantics.
+llvm::APFloat Tmp = llvm::APFloat::getAllOnesValue(FloatingSize, true);
+RepresentsUntilExp = llvm::APFloat::semanticsPrecision(Tmp.getSemantics());
+ }
+
+ if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) {
+// Avoid overflow in our later calculations.
return false;
+ }
+
+ unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+ if (SubType->isSignedIntegerType())
+CorrectedSrcWidth--;
- unsigned W = C.getASTContext().getIntWidth(DestType);
- if (W == 1 || W >= 64U)
+ if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
return false;
+ }
- unsigned long long MaxVal = 1ULL << W;
+ unsigned long long MaxVal = 1ULL << Repr
[PATCH] D52730: [analyzer] ConversionChecker: handle floating point
xazax.hun accepted this revision. xazax.hun added a comment. This revision is now accepted and ready to land. LGTM! I only wonder if this should be on by default or guarded by a config option. I do not have strong feelings about any of the options though. Repository: rC Clang https://reviews.llvm.org/D52730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345594 - [clang] Move two utility functions into SourceManager
Author: lebedevri
Date: Tue Oct 30 05:37:16 2018
New Revision: 345594
URL: http://llvm.org/viewvc/llvm-project?rev=345594&view=rev
Log:
[clang] Move two utility functions into SourceManager
Summary: So we can keep that not-so-great logic in one place.
Reviewers: rsmith, aaron.ballman
Reviewed By: rsmith
Subscribers: nemanjai, kbarton, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D53837
Modified:
cfe/trunk/include/clang/Basic/SourceManager.h
cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
Modified: cfe/trunk/include/clang/Basic/SourceManager.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/SourceManager.h?rev=345594&r1=345593&r2=345594&view=diff
==
--- cfe/trunk/include/clang/Basic/SourceManager.h (original)
+++ cfe/trunk/include/clang/Basic/SourceManager.h Tue Oct 30 05:37:16 2018
@@ -1428,6 +1428,18 @@ public:
return getFileID(Loc) == getMainFileID();
}
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInBuiltinFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInCommandLineFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
/// Returns if a SourceLocation is in a system header.
bool isInSystemHeader(SourceLocation Loc) const {
return isSystem(getFileCharacteristic(Loc));
Modified: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp?rev=345594&r1=345593&r2=345594&view=diff
==
--- cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp (original)
+++ cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp Tue Oct 30 05:37:16 2018
@@ -88,16 +88,6 @@ SourceLocation MacroPPCallbacks::getCorr
return SourceLocation();
}
-static bool isBuiltinFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
-static bool isCommandLineFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
void MacroPPCallbacks::updateStatusToNextScope() {
switch (Status) {
case NoScope:
@@ -127,7 +117,7 @@ void MacroPPCallbacks::FileEntered(Sourc
updateStatusToNextScope();
return;
case BuiltinScope:
-if (isCommandLineFile(PP.getSourceManager(), Loc))
+if (PP.getSourceManager().isWrittenInCommandLineFile(Loc))
return;
updateStatusToNextScope();
LLVM_FALLTHROUGH;
@@ -147,7 +137,7 @@ void MacroPPCallbacks::FileExited(Source
default:
llvm_unreachable("Do not expect to exit a file from current scope");
case BuiltinScope:
-if (!isBuiltinFile(PP.getSourceManager(), Loc))
+if (!PP.getSourceManager().isWrittenInBuiltinFile(Loc))
// Skip next scope and change status to MainFileScope.
Status = MainFileScope;
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53837: [clang] Move two utility functions into SourceManager
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345594: [clang] Move two utility functions into
SourceManager (authored by lebedevri, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53837?vs=171582&id=171681#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53837
Files:
cfe/trunk/include/clang/Basic/SourceManager.h
cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
Index: cfe/trunk/include/clang/Basic/SourceManager.h
===
--- cfe/trunk/include/clang/Basic/SourceManager.h
+++ cfe/trunk/include/clang/Basic/SourceManager.h
@@ -1428,6 +1428,18 @@
return getFileID(Loc) == getMainFileID();
}
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInBuiltinFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInCommandLineFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
/// Returns if a SourceLocation is in a system header.
bool isInSystemHeader(SourceLocation Loc) const {
return isSystem(getFileCharacteristic(Loc));
Index: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
===
--- cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
+++ cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
@@ -88,16 +88,6 @@
return SourceLocation();
}
-static bool isBuiltinFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
-static bool isCommandLineFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
void MacroPPCallbacks::updateStatusToNextScope() {
switch (Status) {
case NoScope:
@@ -127,7 +117,7 @@
updateStatusToNextScope();
return;
case BuiltinScope:
-if (isCommandLineFile(PP.getSourceManager(), Loc))
+if (PP.getSourceManager().isWrittenInCommandLineFile(Loc))
return;
updateStatusToNextScope();
LLVM_FALLTHROUGH;
@@ -147,7 +137,7 @@
default:
llvm_unreachable("Do not expect to exit a file from current scope");
case BuiltinScope:
-if (!isBuiltinFile(PP.getSourceManager(), Loc))
+if (!PP.getSourceManager().isWrittenInBuiltinFile(Loc))
// Skip next scope and change status to MainFileScope.
Status = MainFileScope;
return;
Index: cfe/trunk/include/clang/Basic/SourceManager.h
===
--- cfe/trunk/include/clang/Basic/SourceManager.h
+++ cfe/trunk/include/clang/Basic/SourceManager.h
@@ -1428,6 +1428,18 @@
return getFileID(Loc) == getMainFileID();
}
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInBuiltinFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
+ /// Returns whether \p Loc is located in a file.
+ bool isWrittenInCommandLineFile(SourceLocation Loc) const {
+StringRef Filename(getPresumedLoc(Loc).getFilename());
+return Filename.equals("");
+ }
+
/// Returns if a SourceLocation is in a system header.
bool isInSystemHeader(SourceLocation Loc) const {
return isSystem(getFileCharacteristic(Loc));
Index: cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
===
--- cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
+++ cfe/trunk/lib/CodeGen/MacroPPCallbacks.cpp
@@ -88,16 +88,6 @@
return SourceLocation();
}
-static bool isBuiltinFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
-static bool isCommandLineFile(SourceManager &SM, SourceLocation Loc) {
- StringRef Filename(SM.getPresumedLoc(Loc).getFilename());
- return Filename.equals("");
-}
-
void MacroPPCallbacks::updateStatusToNextScope() {
switch (Status) {
case NoScope:
@@ -127,7 +117,7 @@
updateStatusToNextScope();
return;
case BuiltinScope:
-if (isCommandLineFile(PP.getSourceManager(), Loc))
+if (PP.getSourceManager().isWrittenInCommandLineFile(Loc))
return;
updateStatusToNextScope();
LLVM_FALLTHROUGH;
@@ -147,7 +137,7 @@
default:
llvm_unreachable("Do not expect to exit a file from current scope");
case BuiltinScope:
-if (!isBuiltinFile(PP.getSourceManager(), Loc))
+if (!PP.getSourceManager().isWrittenInBuiltinFile(Loc))
// Skip next scope and change status to MainFileScope.
Status = MainFileScope;
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm
[PATCH] D53837: [clang] Move two utility functions into SourceManager
lebedev.ri added a comment. In https://reviews.llvm.org/D53837#1279891, @rsmith wrote: > Thanks, LG. I bet there's a bunch more places we could change to call these > two. Thank you for the review. Repository: rL LLVM https://reviews.llvm.org/D53837 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52730: [analyzer] ConversionChecker: handle floating point
donat.nagy marked 7 inline comments as done.
donat.nagy added a comment.
I found a solution for determining the precision value of a floating point
type. Is this acceptable?
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:163
+
+switch (FloatingSize) {
+ case 64:
NoQ wrote:
> donat.nagy wrote:
> > NoQ wrote:
> > > Continuing the float semantics discussion on the new revision - Did you
> > > consider `llvm::APFloat`?
> > > (http://llvm.org/doxygen/classllvm_1_1APFloat.html) This looks like
> > > something that you're trying to re-implement.
> > This switch statement essentially fulfills two functions: it maps QualTypes
> > to standardized IEEE floating point types and it immediately maps the
> > standardized IEEE types to their precision values.
> >
> > The difficulty is that I don't think that the first map is available as a
> > public function in the clang libraries. This means that although a switch
> > over the bit width of the floating point type is certainly inelegant, I
> > cannot avoid it.
> >
> > The second map is available in the APFloat library (via the
> > llvm::fltSemantics constants, which describe the standard IEEE types).
> > However, this map is extremely stable (e.g. I don't think that the binary
> > structure of the IEEE double type will ever change), so I think that
> > re-implementing it is justified by the fact that it yields shorter and
> > cleaner code. However, as I had noted previously, I am open to using the
> > llvm::fltSemantics constants to implement this mapping.
> >
> > As an additional remark, my current code supports the _Float16 type, which
> > is currently not supported by the APFloat/fltSemantics library. If we
> > decide to use the llvm::fltSemantics constants, then we must either extend
> > the APFloat library or apply some workarounds for this case.
> >
> > The difficulty is that I don't think that the first map is available as a
> > public function in the clang libraries. This means that although a switch
> > over the bit width of the floating point type is certainly inelegant, I
> > cannot avoid it.
>
> `fltSemantics` are not just enum constants, [[
> http://llvm.org/doxygen/structllvm_1_1fltSemantics.html | they contain a lot
> of fancy data ]]:
> ```
>static const fltSemantics semIEEEhalf = {15, -14, 11, 16};
>static const fltSemantics semIEEEsingle = {127, -126, 24, 32};
>static const fltSemantics semIEEEdouble = {1023, -1022, 53, 64};
>static const fltSemantics semIEEEquad = {16383, -16382, 113, 128};
>static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};
> ```
I knew about the data members of fltSemantics, but I did not see a way to use
them elegantly (although fltSemantics has sizeInBits as a member, the possible
fltSemantics values are not collected into one data structure, so I could not
look up the precision field).
However, as I examined the APFloat.h source again, I found the function
llvm::APFloat::getAllOnesValue which allows me to map bit width to semantics.
While this is a somewhat indirect solution, it avoids using a switch.
The `missing _Float16' problem mentioned in my previous comments was my
mistake, somehow I managed to overlook semIEEEhalf.
Repository:
rC Clang
https://reviews.llvm.org/D52730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53809: Fix invalid address space generation for clk_event_t
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM. Thanks! Could you please add prefix `[OpenCL]` in the final commit title. Repository: rC Clang https://reviews.llvm.org/D53809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
lebedev.ri updated this revision to Diff 171683.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
Better option name.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53817
Files:
clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
clang-tidy/cppcoreguidelines/MacroUsageCheck.h
docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp
test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
test/clang-tidy/cppcoreguidelines-macro-usage.cpp
Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- test/clang-tidy/cppcoreguidelines-macro-usage.cpp
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -4,15 +4,15 @@
#define INCLUDE_GUARD
#define PROBLEMATIC_CONSTANT 0
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC' used; consider using a 'constexpr' variadic template function
#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -6,16 +6,16 @@
#define INCLUDE_GUARD
#define PROBLEMATIC_CONSTANT 0
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC' used; consider using a 'constexpr' variadic template function
#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
-// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
#define DEBUG_CONSTANT 0
#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
Index: test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp
@@ -0,0 +1,8 @@
+// RUN: %check_clang_tidy -check-suffixes=NORMAL %s cppcoreguidelines-macro-usage %t -- -- -D_ZZZ_IM_A_MACRO
+// RUN: %check_clang_tidy -check-suffixes=NORMAL %s cppcoreguidelines-macro-usage %t -- -config='{CheckOptions: [{key: cppcoreguidelines-macro-usage.IgnoreCommandLineMacros, value: 1}]}' -- -D_ZZZ_IM_A_MACRO
+// RUN: %check_clang_tidy -check-suffixes=NORMAL,CL %s cppcoreguidelines-macro-usage %t -- -config='{CheckOptions: [{key: cppcoreguidelines-macro-usage.IgnoreCommandLineMacros, value: 0}]}' -- -D_ZZZ_IM_A_MACRO
+
+// CHECK-MESSAGES-CL: warning: macro '_ZZZ_IM_A_MACRO' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES-NORMAL: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- test/clang-tidy/cppco
[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.
ioeric updated this revision to Diff 171684.
ioeric marked 11 inline comments as done.
ioeric added a comment.
- address review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53433
Files:
clangd/index/Background.cpp
clangd/index/Background.h
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/SyncAPI.cpp
unittests/clangd/SyncAPI.h
Index: unittests/clangd/SyncAPI.h
===
--- unittests/clangd/SyncAPI.h
+++ unittests/clangd/SyncAPI.h
@@ -52,6 +52,7 @@
SymbolSlab runFuzzyFind(const SymbolIndex &Index, StringRef Query);
SymbolSlab runFuzzyFind(const SymbolIndex &Index, const FuzzyFindRequest &Req);
+RefSlab getRefs(const SymbolIndex &Index, SymbolID ID);
} // namespace clangd
} // namespace clang
Index: unittests/clangd/SyncAPI.cpp
===
--- unittests/clangd/SyncAPI.cpp
+++ unittests/clangd/SyncAPI.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "SyncAPI.h"
+#include "index/Index.h"
using namespace llvm;
namespace clang {
@@ -138,5 +139,14 @@
return std::move(Builder).build();
}
+RefSlab getRefs(const SymbolIndex &Index, SymbolID ID) {
+ RefsRequest Req;
+ Req.IDs = {ID};
+ RefSlab::Builder Slab;
+ Index.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
+ return std::move(Slab).build();
+}
+
+
} // namespace clangd
} // namespace clang
Index: unittests/clangd/FileIndexTests.cpp
===
--- unittests/clangd/FileIndexTests.cpp
+++ unittests/clangd/FileIndexTests.cpp
@@ -14,6 +14,7 @@
#include "TestFS.h"
#include "TestTU.h"
#include "index/FileIndex.h"
+#include "index/Index.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/PCHContainerOperations.h"
#include "clang/Frontend/Utils.h"
@@ -39,6 +40,7 @@
}
MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
MATCHER_P(DeclURI, U, "") { return arg.CanonicalDeclaration.FileURI == U; }
+MATCHER_P(DefURI, U, "") { return arg.Definition.FileURI == U; }
MATCHER_P(QName, N, "") { return (arg.Scope + arg.Name).str() == N; }
using namespace llvm;
@@ -73,14 +75,6 @@
return llvm::make_unique(std::move(Slab).build());
}
-RefSlab getRefs(const SymbolIndex &I, SymbolID ID) {
- RefsRequest Req;
- Req.IDs = {ID};
- RefSlab::Builder Slab;
- I.refs(Req, [&](const Ref &S) { Slab.insert(ID, S); });
- return std::move(Slab).build();
-}
-
TEST(FileSymbolsTest, UpdateAndGet) {
FileSymbols FS;
EXPECT_THAT(runFuzzyFind(*FS.buildIndex(IndexType::Light), ""), IsEmpty());
@@ -102,6 +96,27 @@
QName("4"), QName("5")));
}
+TEST(FileSymbolsTest, MergeOverlap) {
+ FileSymbols FS;
+ auto OneSymboSlab = [](Symbol Sym) {
+SymbolSlab::Builder S;
+S.insert(Sym);
+return make_unique(std::move(S).build());
+ };
+ auto X1 = symbol("x");
+ X1.CanonicalDeclaration.FileURI = "file:///x1";
+ auto X2 = symbol("x");
+ X2.Definition.FileURI = "file:///x2";
+
+ FS.update("f1", OneSymboSlab(X1), nullptr);
+ FS.update("f2", OneSymboSlab(X2), nullptr);
+ for (auto Type : {IndexType::Light, IndexType::Heavy})
+EXPECT_THAT(
+runFuzzyFind(*FS.buildIndex(Type, DuplicateHandling::Merge), "x"),
+UnorderedElementsAre(
+AllOf(QName("x"), DeclURI("file:///x1"), DefURI("file:///x2";
+}
+
TEST(FileSymbolsTest, SnapshotAliveAfterRemove) {
FileSymbols FS;
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -4,33 +4,76 @@
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+using testing::_;
+using testing::AllOf;
+using testing::Not;
using testing::UnorderedElementsAre;
namespace clang {
namespace clangd {
MATCHER_P(Named, N, "") { return arg.Name == N; }
+MATCHER(Declared, "") { return !arg.CanonicalDeclaration.FileURI.empty(); }
+MATCHER(Defined, "") { return !arg.Definition.FileURI.empty(); }
+
+MATCHER_P(FileURI, F, "") { return arg.Location.FileURI == F; }
+testing::Matcher
+RefsAre(std::vector> Matchers) {
+ return ElementsAre(testing::Pair(_, UnorderedElementsAreArray(Matchers)));
+}
TEST(BackgroundIndexTest, IndexTwoFiles) {
MockFSProvider FS;
// a.h yields different symbols when included by A.cc vs B.cc.
- // Currently we store symbols for each TU, so we get both.
- FS.Files[testPath("root/A.h")] = "void a_h(); void NAME(){}";
- FS.Files[testPath("root/A.cc")] = "#include \"A.h\"";
- FS.Files[testPath("root/B.cc")] = "#define NAME bar\n#include \"A.h\"";
- BackgroundIndex Idx(Context::empty()
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
lebedev.ri added inline comments. Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:30 + +.. option:: IgnoreLocationless + JonasToth wrote: > I think the another option name would fit better: how about > `IgnoreCommandLineMacros` or the like? Yep, i didn't like that name anyway :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.
ioeric added inline comments.
Comment at: clangd/index/Background.cpp:311
SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
- // FIXME: partition the symbols by file rather than TU, to avoid duplication.
- IndexedSymbols.update(AbsolutePath,
-llvm::make_unique(std::move(Symbols)),
-llvm::make_unique(std::move(Refs)));
- FileHash[AbsolutePath] = Hash;
+ update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate);
+ IndexedFileDigests[AbsolutePath] = Hash;
sammccall wrote:
> It looks like you never write the FilesToUpdate hashes back to the
> IndexedFileDigests, so we'll always update headers?
It's updated in `update` when slabs are updated.
Comment at: clangd/index/Background.h:55
+ using FileDigest = decltype(llvm::SHA1::hash({}));
+ using FileDigests = llvm::StringMap;
sammccall wrote:
> private.
These are also used in some helpers in the cpp file. I can make these private
and make those helper members if you think it would be better?
Comment at: clangd/index/FileIndex.cpp:140
+ }
+ case DuplicateHandling::PickOne: {
+for (const auto &Slab : SymbolSlabs)
sammccall wrote:
> since we're deduplicating above, we could deduplicate here too and remove the
> deduplicating logic from Dex (MemIndex gets it by mistake). That would avoid
> duplicated deduplication (!) in the Merge + Dex case, and seems a little
> cleaner.
>
> Not something for this patch, but maybe add a FIXME.
Added the duduplication for the PickOne case.
Comment at: clangd/index/SymbolCollector.cpp:442
+ auto DefLoc = MI->getDefinitionLoc();
+ // FIXME: use the result to filter out symbols.
+ shouldIndexFile(SM, SM.getFileID(Loc), Opts, &FilesToIndexCache);
sammccall wrote:
> just to check - this is basically trivial to do now but would need tests etc?
> (fine to defer from this patch)
Do you mean just do this for macros for now? Yes, changes for macros should be
trivial.
Comment at: clangd/index/SymbolCollector.h:78
+/// `FileFilter(SM, FID)` is true. If not set, all files are indexed.
+std::function FileFilter = nullptr;
};
sammccall wrote:
> I think we should explicitly test this in SymbolCollectorTests - I think it
> should be straightforward?
This should be trivial for macros, but we still need some refactoring to make
it work for symbols (e.g. `addDeclaration` doesn't support dropping symbols).
Comment at: unittests/clangd/BackgroundIndexTests.cpp:33
+ #if A
+class A_H {};
+ #else
sammccall wrote:
> naming here seems confusing: A_H seems to refer to the header, but then what
> does B_H refer to?
Changed to A_CC and B_CC
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53433
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53433: [clangd] auto-index stores symbols per-file instead of per-TU.
sammccall accepted this revision.
sammccall added inline comments.
Comment at: clangd/index/Background.cpp:311
SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
- // FIXME: partition the symbols by file rather than TU, to avoid duplication.
- IndexedSymbols.update(AbsolutePath,
-llvm::make_unique(std::move(Symbols)),
-llvm::make_unique(std::move(Refs)));
- FileHash[AbsolutePath] = Hash;
+ update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate);
+ IndexedFileDigests[AbsolutePath] = Hash;
ioeric wrote:
> sammccall wrote:
> > It looks like you never write the FilesToUpdate hashes back to the
> > IndexedFileDigests, so we'll always update headers?
> It's updated in `update` when slabs are updated.
In that case, do we need to update IndexedFileDigests here too?
(I just realized there's an edge case: what if there are no symbols or
references in a file? but I'm not sure if this is different between mainfile
and headers, either)
Comment at: clangd/index/Background.h:55
+ using FileDigest = decltype(llvm::SHA1::hash({}));
+ using FileDigests = llvm::StringMap;
ioeric wrote:
> sammccall wrote:
> > private.
> These are also used in some helpers in the cpp file. I can make these private
> and make those helper members if you think it would be better?
oops, true. I'd suggest splitting the difference and keeping FileDigest public
and dropping the FileDigests alias altogether, but up to you.
Comment at: clangd/index/FileIndex.cpp:140
+ }
+ case DuplicateHandling::PickOne: {
+for (const auto &Slab : SymbolSlabs)
ioeric wrote:
> sammccall wrote:
> > since we're deduplicating above, we could deduplicate here too and remove
> > the deduplicating logic from Dex (MemIndex gets it by mistake). That would
> > avoid duplicated deduplication (!) in the Merge + Dex case, and seems a
> > little cleaner.
> >
> > Not something for this patch, but maybe add a FIXME.
> Added the duduplication for the PickOne case.
Thanks! The FIXME is still warranted, to remove deduping from Dex.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53433
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43783: [OpenCL] Remove block invoke function from emitted block literal struct
Anastasia added a comment.
In https://reviews.llvm.org/D43783#1272001, @AlexeySotkin wrote:
> In https://reviews.llvm.org/D43783#1215573, @Anastasia wrote:
>
> > In https://reviews.llvm.org/D43783#1212485, @yaxunl wrote:
> >
> > > In https://reviews.llvm.org/D43783#1204353, @svenvh wrote:
> > >
> > > > Sorry for digging up an old commit...
> > > >
> > > > Apparently this broke block arguments, e.g. the following test case:
> > > >
> > > > int foo(int (^ bl)(void)) {
> > > > return bl();
> > > > }
> > > >
> > > > int get21() {
> > > > return foo(^{return 21;});
> > > > }
> > > >
> > > > int get42() {
> > > > return foo(^{return 42;});
> > > > }
> > > >
> > > >
> > > > In particular, the VarDecl that `getBlockExpr()` sees doesn't have an
> > > > initializer when the called block comes from an argument (causing clang
> > > > to crash).
> > >
> > >
> > > Sorry for the delay. I think block should not be allowed as function
> > > argument since this generally leads indirect function calls therefore
> > > requires support of function pointer. It will rely on optimizations to
> > > get rid of indirect function calls.
> >
> >
> > The idea was to allow blocks as function parameters because they are
> > statically known at each function call. This can be resolved later at IR
> > level instead of frontend. I am also not sure there can be other corner
> > cases where it wouldn't work in Clang since we can't leverage analysis
> > passes here. I feel this might be a risky thing to do in Clang. Currently
> > it doesn't work for the examples provided and therefore breaking the
> > compliance with the spec.
>
>
> The spec is not clear about what is "statically determinable". To me, in the
> example provided we can not resolve the `bl()` call without inlining `foo`,
> even at IR level. As Sam noted, that leads to indirect call and require
> support of functions pointers.
> I see a contradiction in the spec in allowing blocks to be a function
> argument and disallowing function pointers at the same time. I think maybe
> the spec should be changed to clarify existing restrictions or add more
> restrictions for cases when blocks are passed as function argument.
I am not sure that blocks in function call parameters contradict the rule of
them being statically determinable though. You can statically determine what
blocks are being passed into a function. There might be multiple different ones
though. However, it should be possible to lower the function pointers into the
direct calls.
Repository:
rC Clang
https://reviews.llvm.org/D43783
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53814: Allow the analyzer to output to a SARIF file
aaron.ballman marked 9 inline comments as done.
aaron.ballman added inline comments.
Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"
george.karpenkov wrote:
> aaron.ballman wrote:
> > george.karpenkov wrote:
> > > Would it make more sense to just use `diff` + json pretty-formatter to
> > > write a test?
> > > With this test I can't even quite figure out how the output should look
> > > like.
> > I'm not super comfortable with that approach, but perhaps I'm thinking of
> > something different than what you're proposing. The reason I went with this
> > approach is because diff would be fragile (depends heavily on field
> > ordering, which the JSON support library doesn't give control over) and the
> > physical layout of the file isn't what needs to be tested anyway. SARIF has
> > a fair amount of optional data that can be provided as well, so using a
> > purely textual diff worried me that exporting additional optional data in
> > the future would require extensive unrelated changes to all SARIF diffs in
> > the test directory.
> >
> > The goal for this test was to demonstrate that we can validate that the
> > interesting bits of information are present in the output without worrying
> > about the details.
> >
> > Also, the python approach allows us to express relationships between data
> > items more easily than a textual diff tool would. I've not used that here,
> > but I could imagine a test where we want to check that each code location
> > has a corresponding file entry in another list.
> Using a sample file + diff would have an advantage of being easier to read
> (just glance at the "Inputs/blah.serif" to see a sample output), and
> consistent with how we already do checking for plists.
>
> > depends heavily on field ordering
>
> Is it an issue in practice though? I would assume that JSON support library
> would not switch field ordering too often (and even if it does, we can have a
> python wrapper testing that)
>
> > SARIF has a fair amount of optional data
>
> Would diff `--ignore-matching-lines` be enough for those?
Diff testing was what I originally tried and I abandoned it because it was not
viable. The optional data cannot be handled by ignoring matching lines because
the lines won't match from system to system. For instance, there are absolute
URIs to files included in the output, clang git hashes and version numbers that
will change from revision to revision, etc.
What you see as an advantage, I see as more of a distraction -- the actual
layout of the information in the file isn't that important so long as it
validates as valid SARIF (unfortunately, there are no cross-platform command
line tools to validate SARIF yet). What is important are just a few pieces of
the content information (where are the diagnostics, what source ranges and
paths are involved, etc) compared to the overall whole.
I can give diff testing another shot, but I was unable to find a way to make
`-I` work so that I could ignore everything that needs to be ignored due to
natural variance. Do you have ideas there?
To give you an idea of the ultimate form of the output:
```
{
"$schema": "http://json.schemastore.org/sarif-2.0.0";,
"runs": [
{
"files": {
"file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c": {
"fileLocation": {
"uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
},
"length": 3850,
"mimeType": "text/plain",
"roles": [
"resultFile"
]
}
},
"results": [
{
"codeFlows": [
{
"threadFlows": [
{
"locations": [
{
"importance": "essential",
"location": {
"message": {
"text": "Calling 'f'"
},
"physicalLocation": {
"fileLocation": {
"uri":
"file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
},
"region": {
"endColumn": 5,
"endLine": 119,
"startColumn": 3,
"startLine": 119
}
}
},
"step": 1
},
{
"importance": "essential",
"location": {
"message": {
"text": "tainted"
},
"physicalLocation": {
"fileL
[PATCH] D53814: Allow the analyzer to output to a SARIF file
aaron.ballman updated this revision to Diff 171685.
aaron.ballman added a comment.
Updated based on review feedback -- removed the `Sarif` path generation scheme
as it isn't currently needed, and replaced a FIXME with a better comment.
Testing remains an open question, however.
https://reviews.llvm.org/D53814
Files:
Analysis/diagnostics/sarif-check.py
Analysis/diagnostics/sarif-diagnostics-taint-test.c
StaticAnalyzer/Core/CMakeLists.txt
StaticAnalyzer/Core/SarifDiagnostics.cpp
clang/StaticAnalyzer/Core/Analyses.def
clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
Index: Analysis/diagnostics/sarif-diagnostics-taint-test.c
===
--- /dev/null
+++ Analysis/diagnostics/sarif-diagnostics-taint-test.c
@@ -0,0 +1,32 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o %t.sarif | %python %S/sarif-check.py %s %t.sarif
+#include "../Inputs/system-header-simulator.h"
+
+int atoi(const char *nptr);
+
+void f(void) {
+ char s[80];
+ scanf("%s", s);
+ int d = atoi(s); // expected-warning {{tainted}}
+}
+
+int main(void) {
+ f();
+ return 0;
+}
+
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"
+// CHECK: sarifLog['runs'][0]['tool']['name'] == "clang"
+// CHECK: sarifLog['runs'][0]['tool']['language'] == "en-US"
+
+// Test the specifics of this taint test.
+// CHECK: len(result(0)['codeFlows'][0]['threadFlows'][0]['locations']) == 2
+// CHECK: flow(0)['locations'][0]['step'] == 1
+// CHECK: flow(0)['locations'][0]['importance'] == "essential"
+// CHECK: flow(0)['locations'][0]['location']['message']['text'] == "Calling 'f'"
+// CHECK: flow(0)['locations'][0]['location']['physicalLocation']['region']['startLine'] == 13
+// CHECK: flow(0)['locations'][1]['step'] == 2
+// CHECK: flow(0)['locations'][1]['importance'] == "essential"
+// CHECK: flow(0)['locations'][1]['location']['message']['text'] == "tainted"
+// CHECK: flow(0)['locations'][1]['location']['physicalLocation']['region']['startLine'] == 9
Index: Analysis/diagnostics/sarif-check.py
===
--- /dev/null
+++ Analysis/diagnostics/sarif-check.py
@@ -0,0 +1,61 @@
+#!/usr/bin/env python
+
+import sys
+import subprocess
+import traceback
+import json
+
+testfile = sys.argv[1]
+with open(sys.argv[2]) as datafh:
+data = json.load(datafh)
+
+prefix = "CHECK: "
+
+def sarifLogFirstResult(idx):
+return data['runs'][0]['results'][idx]
+
+def threadFlow(idx):
+return data['runs'][0]['results'][0]['codeFlows'][0]['threadFlows'][idx]
+
+fails = 0
+passes = 0
+with open(testfile) as testfh:
+lineno = 0
+for line in iter(testfh.readline, ""):
+lineno += 1
+line = line.rstrip("\r\n")
+try:
+prefix_pos = line.index(prefix)
+except ValueError:
+continue
+check_expr = line[prefix_pos + len(prefix):]
+
+try:
+exception = None
+result = eval(check_expr, {"sarifLog":data,
+ "result":sarifLogFirstResult,
+ "flow":threadFlow})
+except Exception:
+result = False
+exception = traceback.format_exc().splitlines()[-1]
+
+if exception is not None:
+sys.stderr.write(
+"{file}:{line:d}: check threw exception: {expr}\n"
+"{file}:{line:d}: exception was: {exception}\n".format(
+file=testfile, line=lineno,
+expr=check_expr, exception=exception))
+fails += 1
+elif not result:
+sys.stderr.write(
+"{file}:{line:d}: check returned False: {expr}\n".format(
+file=testfile, line=lineno, expr=check_expr))
+fails += 1
+else:
+passes += 1
+
+if fails != 0:
+sys.exit("{} checks failed".format(fails))
+else:
+sys.stdout.write("{} checks passed\n".format(passes))
+
Index: StaticAnalyzer/Core/SarifDiagnostics.cpp
===
--- /dev/null
+++ StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -0,0 +1,270 @@
+//===--- SarifDiagnostics.cpp - Sarif Diagnostics for Paths -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// This file defines the SarifDiagnostics object.
+//
+//===--===//
+
+#include "clang/Basic/Version.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
+#inc
[PATCH] D53025: [clang-tidy] implement new check for const return types.
ymandel marked 2 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<< Def->getReturnType(); aaron.ballman wrote: > ymandel wrote: > > aaron.ballman wrote: > > > ymandel wrote: > > > > aaron.ballman wrote: > > > > > It seems strange to me that this is in the readability module but the > > > > > diagnostic here is about compiler optimizations. Should this be in > > > > > the performance module instead? Or should this diagnostic be reworded > > > > > about the readability concerns? > > > > Good point. The readability angle is that the const clutters the code > > > > to no benefit. That is, even if it was performance neutral, I'd still > > > > want to discourage this practice. But, it does seem like the primary > > > > impact is performance. > > > > > > > > I'm fine either way -- moving it to performance or emphasizing the > > > > clutter of the unhelpful "const". I'm inclined to moving it, but don't > > > > have good sense of how strict these hierarchies are. What do you think? > > > I'm not sold that `const`-qualified return types always pessimize > > > optimizations. However, I'm also not sold that it's *always* a bad idea > > > to have a top-level cv-qualifier on a return type either (for instance, > > > this is one way to prevent callers from modifying a temp returned by a > > > function). How about leaving this in readability and changing the > > > diagnostic into: "top-level 'const' qualifier on a return type may reduce > > > code readability with limited benefit" or something equally weasely? > > I talked this over with other google folks and seems like the consensus is: > > > > 1. The readability benefits may be controversial. Some folks might not > > view `const` as clutter and there are some corner cases where the qualifier > > may prevent something harmful. > > 2. The performance implication is real, if not guaranteed to be a problem. > > > > Based on this, seems best to move it to performance, but water down the > > performance claims. That keeps the focus to an issue we can assume nearly > > everyone agrees on. > I'm not convinced the performance implications are real compared to how the > check is currently implemented. I know there are performance concerns when > you return a const value of class type because it pessimizes the ability to > use move constructors, but that's a very specific performance concern that I > don't believe is present in general. For instance, I don't know of any > performance concerns regarding `const int f();` as a declaration. I'd be fine > moving this to the performance module, but I think the check would need to be > tightened to only focus on actual performance concerns. > > FWIW, I do agree that the readability benefits may be controversial, but I > kind of thought that was the point to the check and as such, it's a very > reasonable check. Almost everything in readability is subjective to some > degree and our metric has always been "if you agree with a style check, don't > enable it". > > It's up to you how you want to proceed, but I see two ways forward: 1) keep > this as a readability check that broadly finds top-level const qualifiers and > removes them, 2) move this to the performance module and rework the check to > focus on only the scenarios that have concrete performance impact. > It's up to you how you want to proceed, but I see two ways forward: 1) keep > this as a readability check that broadly finds top-level const qualifiers and > removes them, 2) move this to the performance module and rework the check to > focus on only the scenarios that have concrete performance impact. Aaron, thank you for the detailed response. I'm inclined to agree with you that this belongs in readabilty and I spoke with sbenza who feels the same. The high-level point is that the `const` is noise in most cases. You suggested above a warning along the lines of: "top-level 'const' qualifier on a return type may reduce code readability with limited benefit" I like this, but think it should be a little stronger. Perhaps: "top-level 'const' qualifier on a return type may reduce code readability while rarely having an effect" I also propose updating the explanation in the documentation file from: Such use of ``const`` is superfluous, and prevents valuable compiler optimizations. to the weaker Such use of ``const`` is usually superfluous, and can prevent valuable compiler optimizations. WDYT? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345597 - [AST] Only store the needed data in WhileStmt
Author: brunoricci
Date: Tue Oct 30 06:42:41 2018
New Revision: 345597
URL: http://llvm.org/viewvc/llvm-project?rev=345597&view=rev
Log:
[AST] Only store the needed data in WhileStmt
Don't store the data for the condition variable if not needed.
This cuts the size of WhileStmt by up to a pointer.
The order of the children is kept the same.
Differential Revision: https://reviews.llvm.org/D53715
Reviewed By: rjmccall
Modified:
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/ASTDumper.cpp
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/Stmt.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
cfe/trunk/test/Import/while-stmt/test.cpp
Modified: cfe/trunk/include/clang/AST/Stmt.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Stmt.h?rev=345597&r1=345596&r2=345597&view=diff
==
--- cfe/trunk/include/clang/AST/Stmt.h (original)
+++ cfe/trunk/include/clang/AST/Stmt.h Tue Oct 30 06:42:41 2018
@@ -193,10 +193,14 @@ protected:
};
class WhileStmtBitfields {
+friend class ASTStmtReader;
friend class WhileStmt;
unsigned : NumStmtBits;
+/// True if the WhileStmt has storage for a condition variable.
+unsigned HasVar : 1;
+
/// The location of the "while".
SourceLocation WhileLoc;
};
@@ -1615,16 +1619,75 @@ public:
};
/// WhileStmt - This represents a 'while' stmt.
-class WhileStmt : public Stmt {
- enum { VAR, COND, BODY, END_EXPR };
- Stmt* SubExprs[END_EXPR];
+class WhileStmt final : public Stmt,
+private llvm::TrailingObjects {
+ friend TrailingObjects;
-public:
- WhileStmt(const ASTContext &C, VarDecl *Var, Expr *cond, Stmt *body,
+ // WhileStmt is followed by several trailing objects,
+ // some of which optional. Note that it would be more
+ // convenient to put the optional trailing object at the end
+ // but this would affect children().
+ // The trailing objects are in order:
+ //
+ // * A "Stmt *" for the condition variable.
+ //Present if and only if hasVarStorage(). This is in fact a "DeclStmt *".
+ //
+ // * A "Stmt *" for the condition.
+ //Always present. This is in fact an "Expr *".
+ //
+ // * A "Stmt *" for the body.
+ //Always present.
+ //
+ enum { VarOffset = 0, BodyOffsetFromCond = 1 };
+ enum { NumMandatoryStmtPtr = 2 };
+
+ unsigned varOffset() const { return VarOffset; }
+ unsigned condOffset() const { return VarOffset + hasVarStorage(); }
+ unsigned bodyOffset() const { return condOffset() + BodyOffsetFromCond; }
+
+ unsigned numTrailingObjects(OverloadToken) const {
+return NumMandatoryStmtPtr + hasVarStorage();
+ }
+
+ /// Build a while statement.
+ WhileStmt(const ASTContext &Ctx, VarDecl *Var, Expr *Cond, Stmt *Body,
SourceLocation WL);
/// Build an empty while statement.
- explicit WhileStmt(EmptyShell Empty) : Stmt(WhileStmtClass, Empty) {}
+ explicit WhileStmt(EmptyShell Empty, bool HasVar);
+
+public:
+ /// Create a while statement.
+ static WhileStmt *Create(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
+ Stmt *Body, SourceLocation WL);
+
+ /// Create an empty while statement optionally with storage for
+ /// a condition variable.
+ static WhileStmt *CreateEmpty(const ASTContext &Ctx, bool HasVar);
+
+ /// True if this WhileStmt has storage for a condition variable.
+ bool hasVarStorage() const { return WhileStmtBits.HasVar; }
+
+ Expr *getCond() {
+return reinterpret_cast(getTrailingObjects()[condOffset()]);
+ }
+
+ const Expr *getCond() const {
+return reinterpret_cast(getTrailingObjects()[condOffset()]);
+ }
+
+ void setCond(Expr *Cond) {
+getTrailingObjects()[condOffset()] = reinterpret_cast(Cond);
+ }
+
+ Stmt *getBody() { return getTrailingObjects()[bodyOffset()]; }
+ const Stmt *getBody() const {
+return getTrailingObjects()[bodyOffset()];
+ }
+
+ void setBody(Stmt *Body) {
+getTrailingObjects()[bodyOffset()] = Body;
+ }
/// Retrieve the variable declared in this "while" statement, if any.
///
@@ -1634,28 +1697,36 @@ public:
/// // ...
/// }
/// \endcode
- VarDecl *getConditionVariable() const;
- void setConditionVariable(const ASTContext &C, VarDecl *V);
+ VarDecl *getConditionVariable();
+ const VarDecl *getConditionVariable() const {
+return const_cast(this)->getConditionVariable();
+ }
+
+ /// Set the condition variable of this while statement.
+ /// The while statement must have storage for it.
+ void setConditionVariable(const ASTContext &Ctx, VarDecl *V);
/// If this WhileStmt has a condition variable, return the faux DeclStmt
/// associated with the creation of that condition variable.
- const DeclStmt *getConditionVariableDeclStmt() const {
-return reinterpret_cast(SubExprs[VAR]);
+ DeclStmt *getConditionVa
[PATCH] D53715: [AST] Only store the needed data in WhileStmt.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345597: [AST] Only store the needed data in WhileStmt
(authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53715?vs=171338&id=171686#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53715
Files:
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/ASTDumper.cpp
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/Stmt.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
cfe/trunk/test/Import/while-stmt/test.cpp
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -5864,9 +5864,8 @@
SourceLocation ToWhileLoc;
std::tie(ToConditionVariable, ToCond, ToBody, ToWhileLoc) = *Imp;
- return new (Importer.getToContext()) WhileStmt(
- Importer.getToContext(),
- ToConditionVariable, ToCond, ToBody, ToWhileLoc);
+ return WhileStmt::Create(Importer.getToContext(), ToConditionVariable, ToCond,
+ ToBody, ToWhileLoc);
}
ExpectedStmt ASTNodeImporter::VisitDoStmt(DoStmt *S) {
Index: cfe/trunk/lib/AST/Stmt.cpp
===
--- cfe/trunk/lib/AST/Stmt.cpp
+++ cfe/trunk/lib/AST/Stmt.cpp
@@ -978,32 +978,60 @@
DeclStmt(DeclGroupRef(V), VarRange.getBegin(), VarRange.getEnd());
}
-WhileStmt::WhileStmt(const ASTContext &C, VarDecl *Var, Expr *cond, Stmt *body,
- SourceLocation WL)
- : Stmt(WhileStmtClass) {
- setConditionVariable(C, Var);
- SubExprs[COND] = cond;
- SubExprs[BODY] = body;
- WhileStmtBits.WhileLoc = WL;
+WhileStmt::WhileStmt(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
+ Stmt *Body, SourceLocation WL)
+: Stmt(WhileStmtClass) {
+ bool HasVar = Var != nullptr;
+ WhileStmtBits.HasVar = HasVar;
+
+ setCond(Cond);
+ setBody(Body);
+ if (HasVar)
+setConditionVariable(Ctx, Var);
+
+ setWhileLoc(WL);
}
-VarDecl *WhileStmt::getConditionVariable() const {
- if (!SubExprs[VAR])
-return nullptr;
+WhileStmt::WhileStmt(EmptyShell Empty, bool HasVar)
+: Stmt(WhileStmtClass, Empty) {
+ WhileStmtBits.HasVar = HasVar;
+}
+
+WhileStmt *WhileStmt::Create(const ASTContext &Ctx, VarDecl *Var, Expr *Cond,
+ Stmt *Body, SourceLocation WL) {
+ bool HasVar = Var != nullptr;
+ void *Mem =
+ Ctx.Allocate(totalSizeToAlloc(NumMandatoryStmtPtr + HasVar),
+ alignof(WhileStmt));
+ return new (Mem) WhileStmt(Ctx, Var, Cond, Body, WL);
+}
- auto *DS = cast(SubExprs[VAR]);
+WhileStmt *WhileStmt::CreateEmpty(const ASTContext &Ctx, bool HasVar) {
+ void *Mem =
+ Ctx.Allocate(totalSizeToAlloc(NumMandatoryStmtPtr + HasVar),
+ alignof(WhileStmt));
+ return new (Mem) WhileStmt(EmptyShell(), HasVar);
+}
+
+VarDecl *WhileStmt::getConditionVariable() {
+ auto *DS = getConditionVariableDeclStmt();
+ if (!DS)
+return nullptr;
return cast(DS->getSingleDecl());
}
-void WhileStmt::setConditionVariable(const ASTContext &C, VarDecl *V) {
+void WhileStmt::setConditionVariable(const ASTContext &Ctx, VarDecl *V) {
+ assert(hasVarStorage() &&
+ "This while statement has no storage for a condition variable!");
+
if (!V) {
-SubExprs[VAR] = nullptr;
+getTrailingObjects()[varOffset()] = nullptr;
return;
}
SourceRange VarRange = V->getSourceRange();
- SubExprs[VAR] = new (C) DeclStmt(DeclGroupRef(V), VarRange.getBegin(),
- VarRange.getEnd());
+ getTrailingObjects()[varOffset()] = new (Ctx)
+ DeclStmt(DeclGroupRef(V), VarRange.getBegin(), VarRange.getEnd());
}
// IndirectGotoStmt
Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -513,6 +513,7 @@
void VisitAttributedStmt(const AttributedStmt *Node);
void VisitIfStmt(const IfStmt *Node);
void VisitSwitchStmt(const SwitchStmt *Node);
+void VisitWhileStmt(const WhileStmt *Node);
void VisitLabelStmt(const LabelStmt *Node);
void VisitGotoStmt(const GotoStmt *Node);
void VisitCXXCatchStmt(const CXXCatchStmt *Node);
@@ -2041,6 +2042,12 @@
OS << " has_var";
}
+void ASTDumper::VisitWhileStmt(const WhileStmt *Node) {
+ VisitStmt(Node);
+ if (Node->hasVarStorage())
+OS << " has_var";
+}
+
void ASTDumper::VisitLabelStmt(const LabelStmt *Node) {
VisitStmt(Node);
OS << " '" << Node->getName() << "'";
Index: cfe/trunk/lib/Sema/SemaStmt.cpp
===
--- cfe/trunk/lib/Sema/SemaStmt.cpp
+++ cfe/trunk/lib/Sema/SemaStmt.cpp
@@ -1306,8 +1306,8 @@
if
[PATCH] D53025: [clang-tidy] implement new check for const return types.
aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<< Def->getReturnType(); ymandel wrote: > aaron.ballman wrote: > > ymandel wrote: > > > aaron.ballman wrote: > > > > ymandel wrote: > > > > > aaron.ballman wrote: > > > > > > It seems strange to me that this is in the readability module but > > > > > > the diagnostic here is about compiler optimizations. Should this be > > > > > > in the performance module instead? Or should this diagnostic be > > > > > > reworded about the readability concerns? > > > > > Good point. The readability angle is that the const clutters the code > > > > > to no benefit. That is, even if it was performance neutral, I'd > > > > > still want to discourage this practice. But, it does seem like the > > > > > primary impact is performance. > > > > > > > > > > I'm fine either way -- moving it to performance or emphasizing the > > > > > clutter of the unhelpful "const". I'm inclined to moving it, but > > > > > don't have good sense of how strict these hierarchies are. What do > > > > > you think? > > > > I'm not sold that `const`-qualified return types always pessimize > > > > optimizations. However, I'm also not sold that it's *always* a bad idea > > > > to have a top-level cv-qualifier on a return type either (for instance, > > > > this is one way to prevent callers from modifying a temp returned by a > > > > function). How about leaving this in readability and changing the > > > > diagnostic into: "top-level 'const' qualifier on a return type may > > > > reduce code readability with limited benefit" or something equally > > > > weasely? > > > I talked this over with other google folks and seems like the consensus > > > is: > > > > > > 1. The readability benefits may be controversial. Some folks might not > > > view `const` as clutter and there are some corner cases where the > > > qualifier may prevent something harmful. > > > 2. The performance implication is real, if not guaranteed to be a > > > problem. > > > > > > Based on this, seems best to move it to performance, but water down the > > > performance claims. That keeps the focus to an issue we can assume > > > nearly everyone agrees on. > > I'm not convinced the performance implications are real compared to how the > > check is currently implemented. I know there are performance concerns when > > you return a const value of class type because it pessimizes the ability to > > use move constructors, but that's a very specific performance concern that > > I don't believe is present in general. For instance, I don't know of any > > performance concerns regarding `const int f();` as a declaration. I'd be > > fine moving this to the performance module, but I think the check would > > need to be tightened to only focus on actual performance concerns. > > > > FWIW, I do agree that the readability benefits may be controversial, but I > > kind of thought that was the point to the check and as such, it's a very > > reasonable check. Almost everything in readability is subjective to some > > degree and our metric has always been "if you agree with a style check, > > don't enable it". > > > > It's up to you how you want to proceed, but I see two ways forward: 1) keep > > this as a readability check that broadly finds top-level const qualifiers > > and removes them, 2) move this to the performance module and rework the > > check to focus on only the scenarios that have concrete performance impact. > > It's up to you how you want to proceed, but I see two ways forward: 1) keep > > this as a readability check that broadly finds top-level const qualifiers > > and removes them, 2) move this to the performance module and rework the > > check to focus on only the scenarios that have concrete performance impact. > > Aaron, thank you for the detailed response. I'm inclined to agree with you > that this belongs in readabilty and I spoke with sbenza who feels the same. > The high-level point is that the `const` is noise in most cases. > > You suggested above a warning along the lines of: > "top-level 'const' qualifier on a return type may reduce code readability > with limited benefit" > > I like this, but think it should be a little stronger. Perhaps: > "top-level 'const' qualifier on a return type may reduce code readability > while rarely having an effect" > > I also propose updating the explanation in the documentation file from: > > Such use of ``const`` is superfluous, and > prevents valuable compiler optimizations. > > to the weaker > > Such use of ``const`` is usually superfluous, and can > prevent valuable compiler optimizations. > > WDYT? > I like this, but think it should be a little stronger. Perhaps: > "top-level 'const' qual
[PATCH] D50250: [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part
lebedev.ri updated this revision to Diff 171688.
lebedev.ri added a comment.
Rebased, NFC.
Repository:
rC Clang
https://reviews.llvm.org/D50250
Files:
docs/ReleaseNotes.rst
docs/UndefinedBehaviorSanitizer.rst
include/clang/Basic/Sanitizers.def
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/catch-implicit-conversions-basics.c
test/CodeGen/catch-implicit-integer-arithmetic-value-change-basics.c
test/CodeGen/catch-implicit-integer-conversions-basics.c
test/CodeGen/catch-implicit-integer-sign-changes-basics.c
test/CodeGen/catch-implicit-integer-sign-changes-true-negatives.c
test/CodeGen/catch-implicit-integer-sign-changes.c
test/CodeGen/catch-implicit-signed-integer-truncation-or-sign-change.c
test/CodeGenCXX/catch-implicit-integer-sign-changes-true-negatives.cpp
test/Driver/fsanitize.c
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -31,22 +31,37 @@
// CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
// RUN: %clang -target x86_64-linux-gnu -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){7}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){8}"}}
// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fno-sanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-NORECOVER
// RUN: %clang -target x86_64-linux-gnu -fsanitize=implicit-conversion -fsanitize-trap=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-TRAP
-// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}} // ???
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
-// CHECK-implicit-conversion-TRAP-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation),?){2}"}}
+// CHECK-implicit-conversion: "-fsanitize={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-RECOVER-NOT: "-fsanitize-trap={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}}
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fno-sanitize-recover={{((implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){3}"}} // ???
+// CHECK-implicit-conversion-NORECOVER-NOT: "-fsanitize-recover={{((implicit-unsigned-integer-truncation|implici
[PATCH] D53866: [Preamble] Fix preamble for circular #includes
nik created this revision.
Herald added subscribers: cfe-commits, arphaman.
If a header file was processed for the second time, we could end up with
a wrong conditional stack and skipped ranges:
In the particular example, if the header guard is evaluated the second
time and it is decided to skip the conditional block, the corresponding
"#endif" is never seen since the preamble does not include it and we end
up in the Tok.is(tok::eof) case with a wrong conditional stack.
Fix this by resetting the conditional state in such a case.
Repository:
rC Clang
https://reviews.llvm.org/D53866
Files:
lib/Lex/PPDirectives.cpp
test/Index/preamble-cyclic-include.cpp
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,8 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:4:1:8:1
%s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -361,6 +361,14 @@
}
}
+static bool isMainFileIncludedAgain(const SourceManager &sourceManager,
+HeaderSearch &headerSearch,
+const FileEntry *fileEntry) {
+ return sourceManager.translateFile(fileEntry) ==
+ sourceManager.getMainFileID() &&
+ headerSearch.getFileInfo(fileEntry).NumIncludes > 1;
+}
+
/// SkipExcludedConditionalBlock - We just read a \#if or related directive and
/// decided that the subsequent tokens are in the \#if'd out portion of the
/// file. Lex the rest of the file, until we see an \#endif. If
@@ -377,6 +385,8 @@
++NumSkipped;
assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
+ const auto InitialConditionalStack = CurPPLexer->ConditionalStack;
+
if (PreambleConditionalStack.reachedEOFWhileSkipping())
PreambleConditionalStack.clearSkipInfo();
else
@@ -407,9 +417,16 @@
// We don't emit errors for unterminated conditionals here,
// Lexer::LexEndOfFile can do that propertly.
// Just return and let the caller lex after this #include.
- if (PreambleConditionalStack.isRecording())
-PreambleConditionalStack.SkipInfo.emplace(
-HashTokenLoc, IfTokenLoc, FoundNonSkipPortion, FoundElse, ElseLoc);
+ if (PreambleConditionalStack.isRecording()) {
+if (isMainFileIncludedAgain(getSourceManager(), HeaderInfo,
+CurLexer->getFileEntry())) {
+ CurPPLexer->ConditionalStack = InitialConditionalStack;
+} else {
+ PreambleConditionalStack.SkipInfo.emplace(HashTokenLoc, IfTokenLoc,
+FoundNonSkipPortion,
+FoundElse, ElseLoc);
+}
+ }
break;
}
Index: test/Index/preamble-cyclic-include.cpp
===
--- /dev/null
+++ test/Index/preamble-cyclic-include.cpp
@@ -0,0 +1,8 @@
+// RUN: env CINDEXTEST_EDITING=1 c-index-test -test-annotate-tokens=%s:4:1:8:1 %s 2>&1 | FileCheck %s
+// CHECK-NOT: error: unterminated conditional directive
+// CHECK-NOT: Skipping: [4:1 - 8:7]
+#ifndef A_H
+#define A_H
+# include "preamble-cyclic-include.cpp"
+int bar();
+#endif
Index: lib/Lex/PPDirectives.cpp
===
--- lib/Lex/PPDirectives.cpp
+++ lib/Lex/PPDirectives.cpp
@@ -361,6 +361,14 @@
}
}
+static bool isMainFileIncludedAgain(const SourceManager &sourceManager,
+HeaderSearch &headerSearch,
+const FileEntry *fileEntry) {
+ return sourceManager.translateFile(fileEntry) ==
+ sourceManager.getMainFileID() &&
+ headerSearch.getFileInfo(fileEntry).NumIncludes > 1;
+}
+
/// SkipExcludedConditionalBlock - We just read a \#if or related directive and
/// decided that the subsequent tokens are in the \#if'd out portion of the
/// file. Lex the rest of the file, until we see an \#endif. If
@@ -377,6 +385,8 @@
++NumSkipped;
assert(!CurTokenLexer && CurPPLexer && "Lexing a macro, not a file?");
+ const auto InitialConditionalStack = CurPPLexer->ConditionalStack;
+
if (PreambleConditionalStack.reachedEOFWhileSkipping())
PreambleConditionalStack.clearSkipInfo();
else
@@ -407,9 +417,16 @@
// We don't emit errors for unterminated conditionals here,
// Lexer::LexEndOfFile can do that propertly.
// Just return and let the caller lex after this #include.
- if (PreambleConditionalStack.isRecording())
-Preambl
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet updated this revision to Diff 171692.
gchatelet added a comment.
- Add more checks, disable bool implicit casts warning, enable ternary operator
warnings.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53488
Files:
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -- --
+// -target x86_64-unknown-linux
float ceil(float);
namespace std {
@@ -9,12 +10,12 @@
namespace floats {
struct ConvertsToFloat {
- operator float() const { return 0.5; }
+ operator float() const { return 0.5f; }
};
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
int i = 0;
i = d;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -24,11 +25,11 @@
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = ConvertsToFloat();
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i = 15_Pa;
+ i = 15_float;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
}
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
int i = 0;
i += 0.5;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -52,6 +53,129 @@
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
}
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+ return 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok(double d) {
+ float f;
+ f = d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f = 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f = 15_double;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok_binary_ops(double d) {
+ float f;
+ f += 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ // We warn on the following even though it's not dangerous because there is no reason to use a double literal here.
+ // TODO(courbet): Provide an automatic fix.
+ f += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+
+ f *= 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f /= 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f += (double)0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_integer_to_floating() {
+ {
+long long ll; // 64 bits
+float f = ll; // doesn't fit in 24 bits
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'long long' to 'float' [cppcoreguidelines-narrowing-conversions]
+double d = ll; // doesn't fit in 53 bits.
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+ }
+ {
+int i; // 32 bits
+float f = i; // doesn't fit in 24 bits
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions]
+double d = i; // fits in 53 bits.
+ }
+
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM, @aaron.ballman do you have more comments? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53856: [analyzer] Put llvm.Conventions back in alpha
Szelethus added a comment. In https://reviews.llvm.org/D53856#1279887, @NoQ wrote: > This might be also covered by @rnkovacs's string buffer escape checker - > either already or eventually, it'll become just yet another string view API > that the checker supports. I thought about that too, adding some `StringRef` specific information to that checker makes more sense then essentially duplicating the logic. However, what @MTC mentioned about `ArrayRef` would be a neat addition too, and maybe it isn't worth making `InnerPointerChecker` //that// general. @rnkovacs, any thoughts? https://reviews.llvm.org/D53856 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52676: [clang-format] tweaked another case of lambda formatting
djasper added a comment.
I think this roughly looks fine. Krasimir, any thoughts?
Comment at: unittests/Format/FormatTest.cpp:11854
+ // case above.
+ {
+auto Style = getGoogleStyle();
No need to use a scope here. Feel free to redefine Style.
If in fact you feel like that is getting out of hand, maybe extract a separate
TEST_F() for this.
Comment at: unittests/Format/FormatTest.cpp:11865
+ }
+ {
+verifyFormat("SomeFunction(\n"
No need for this scope.
Repository:
rC Clang
https://reviews.llvm.org/D52676
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet added a comment. So I've updated the code to add more narrowing conversions. It now tests constant expressions against receiving type, do not warn about implicit bool casting, handles ternary operator with constant expressions. I ran it on our code base: results look legit. I'll ping back here with numbers for llvm once I figured out how to build the database the tool is complaining about :) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345605 - [AST] Only store data for the NRVO candidate in ReturnStmt if needed
Author: brunoricci
Date: Tue Oct 30 07:40:49 2018
New Revision: 345605
URL: http://llvm.org/viewvc/llvm-project?rev=345605&view=rev
Log:
[AST] Only store data for the NRVO candidate in ReturnStmt if needed
Only store the NRVO candidate if needed in ReturnStmt.
A good chuck of all of the ReturnStmt have no NRVO candidate
(more than half when parsing all of Boost). For all of them
this saves one pointer. This has no impact on children().
Differential Revision: https://reviews.llvm.org/D53716
Reviewed By: rsmith
Modified:
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/Stmt.cpp
cfe/trunk/lib/Analysis/BodyFarm.cpp
cfe/trunk/lib/CodeGen/CGObjC.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
Modified: cfe/trunk/include/clang/AST/Stmt.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Stmt.h?rev=345605&r1=345604&r2=345605&view=diff
==
--- cfe/trunk/include/clang/AST/Stmt.h (original)
+++ cfe/trunk/include/clang/AST/Stmt.h Tue Oct 30 07:40:49 2018
@@ -256,6 +256,9 @@ protected:
unsigned : NumStmtBits;
+/// True if this ReturnStmt has storage for an NRVO candidate.
+unsigned HasNRVOCandidate : 1;
+
/// The location of the "return".
SourceLocation RetLoc;
};
@@ -1999,40 +2002,67 @@ public:
/// return a value, and it allows returning a value in functions declared to
/// return void. We explicitly model this in the AST, which means you can't
/// depend on the return type of the function and the presence of an argument.
-class ReturnStmt : public Stmt {
+class ReturnStmt final
+: public Stmt,
+ private llvm::TrailingObjects {
+ friend TrailingObjects;
+
+ /// The return expression.
Stmt *RetExpr;
- const VarDecl *NRVOCandidate;
-public:
- explicit ReturnStmt(SourceLocation RL) : ReturnStmt(RL, nullptr, nullptr) {}
+ // ReturnStmt is followed optionally by a trailing "const VarDecl *"
+ // for the NRVO candidate. Present if and only if hasNRVOCandidate().
- ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate)
- : Stmt(ReturnStmtClass), RetExpr((Stmt *)E),
-NRVOCandidate(NRVOCandidate) {
-ReturnStmtBits.RetLoc = RL;
+ /// True if this ReturnStmt has storage for an NRVO candidate.
+ bool hasNRVOCandidate() const { return ReturnStmtBits.HasNRVOCandidate; }
+
+ unsigned numTrailingObjects(OverloadToken) const {
+return hasNRVOCandidate();
}
- /// Build an empty return expression.
- explicit ReturnStmt(EmptyShell Empty) : Stmt(ReturnStmtClass, Empty) {}
+ /// Build a return statement.
+ ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate);
- const Expr *getRetValue() const;
- Expr *getRetValue();
- void setRetValue(Expr *E) { RetExpr = reinterpret_cast(E); }
+ /// Build an empty return statement.
+ explicit ReturnStmt(EmptyShell Empty, bool HasNRVOCandidate);
- SourceLocation getReturnLoc() const { return ReturnStmtBits.RetLoc; }
- void setReturnLoc(SourceLocation L) { ReturnStmtBits.RetLoc = L; }
+public:
+ /// Create a return statement.
+ static ReturnStmt *Create(const ASTContext &Ctx, SourceLocation RL, Expr *E,
+const VarDecl *NRVOCandidate);
+
+ /// Create an empty return statement, optionally with
+ /// storage for an NRVO candidate.
+ static ReturnStmt *CreateEmpty(const ASTContext &Ctx, bool HasNRVOCandidate);
+
+ Expr *getRetValue() { return reinterpret_cast(RetExpr); }
+ const Expr *getRetValue() const { return reinterpret_cast(RetExpr); }
+ void setRetValue(Expr *E) { RetExpr = reinterpret_cast(E); }
/// Retrieve the variable that might be used for the named return
/// value optimization.
///
/// The optimization itself can only be performed if the variable is
/// also marked as an NRVO object.
- const VarDecl *getNRVOCandidate() const { return NRVOCandidate; }
- void setNRVOCandidate(const VarDecl *Var) { NRVOCandidate = Var; }
+ const VarDecl *getNRVOCandidate() const {
+return hasNRVOCandidate() ? *getTrailingObjects()
+ : nullptr;
+ }
- SourceLocation getBeginLoc() const { return getReturnLoc(); }
+ /// Set the variable that might be used for the named return value
+ /// optimization. The return statement must have storage for it,
+ /// which is the case if and only if hasNRVOCandidate() is true.
+ void setNRVOCandidate(const VarDecl *Var) {
+assert(hasNRVOCandidate() &&
+ "This return statement has no storage for an NRVO candidate!");
+*getTrailingObjects() = Var;
+ }
- SourceLocation getEndLoc() const {
+ SourceLocation getReturnLoc() const { return ReturnStmtBits.RetLoc; }
+ void setReturnLoc(SourceLocation L) { ReturnStmtBits.RetLoc = L; }
+
+ SourceLocation getBeginLoc() const { return getReturnLoc(); }
[PATCH] D53716: [AST] Only store data for the NRVO candidate in ReturnStmt if needed.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345605: [AST] Only store data for the NRVO candidate in
ReturnStmt if needed (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53716?vs=171131&id=171694#toc
Repository:
rC Clang
https://reviews.llvm.org/D53716
Files:
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/lib/AST/Stmt.cpp
cfe/trunk/lib/Analysis/BodyFarm.cpp
cfe/trunk/lib/CodeGen/CGObjC.cpp
cfe/trunk/lib/Sema/SemaStmt.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
Index: cfe/trunk/include/clang/AST/Stmt.h
===
--- cfe/trunk/include/clang/AST/Stmt.h
+++ cfe/trunk/include/clang/AST/Stmt.h
@@ -256,6 +256,9 @@
unsigned : NumStmtBits;
+/// True if this ReturnStmt has storage for an NRVO candidate.
+unsigned HasNRVOCandidate : 1;
+
/// The location of the "return".
SourceLocation RetLoc;
};
@@ -1999,40 +2002,67 @@
/// return a value, and it allows returning a value in functions declared to
/// return void. We explicitly model this in the AST, which means you can't
/// depend on the return type of the function and the presence of an argument.
-class ReturnStmt : public Stmt {
+class ReturnStmt final
+: public Stmt,
+ private llvm::TrailingObjects {
+ friend TrailingObjects;
+
+ /// The return expression.
Stmt *RetExpr;
- const VarDecl *NRVOCandidate;
-public:
- explicit ReturnStmt(SourceLocation RL) : ReturnStmt(RL, nullptr, nullptr) {}
+ // ReturnStmt is followed optionally by a trailing "const VarDecl *"
+ // for the NRVO candidate. Present if and only if hasNRVOCandidate().
- ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate)
- : Stmt(ReturnStmtClass), RetExpr((Stmt *)E),
-NRVOCandidate(NRVOCandidate) {
-ReturnStmtBits.RetLoc = RL;
+ /// True if this ReturnStmt has storage for an NRVO candidate.
+ bool hasNRVOCandidate() const { return ReturnStmtBits.HasNRVOCandidate; }
+
+ unsigned numTrailingObjects(OverloadToken) const {
+return hasNRVOCandidate();
}
- /// Build an empty return expression.
- explicit ReturnStmt(EmptyShell Empty) : Stmt(ReturnStmtClass, Empty) {}
+ /// Build a return statement.
+ ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate);
- const Expr *getRetValue() const;
- Expr *getRetValue();
- void setRetValue(Expr *E) { RetExpr = reinterpret_cast(E); }
+ /// Build an empty return statement.
+ explicit ReturnStmt(EmptyShell Empty, bool HasNRVOCandidate);
- SourceLocation getReturnLoc() const { return ReturnStmtBits.RetLoc; }
- void setReturnLoc(SourceLocation L) { ReturnStmtBits.RetLoc = L; }
+public:
+ /// Create a return statement.
+ static ReturnStmt *Create(const ASTContext &Ctx, SourceLocation RL, Expr *E,
+const VarDecl *NRVOCandidate);
+
+ /// Create an empty return statement, optionally with
+ /// storage for an NRVO candidate.
+ static ReturnStmt *CreateEmpty(const ASTContext &Ctx, bool HasNRVOCandidate);
+
+ Expr *getRetValue() { return reinterpret_cast(RetExpr); }
+ const Expr *getRetValue() const { return reinterpret_cast(RetExpr); }
+ void setRetValue(Expr *E) { RetExpr = reinterpret_cast(E); }
/// Retrieve the variable that might be used for the named return
/// value optimization.
///
/// The optimization itself can only be performed if the variable is
/// also marked as an NRVO object.
- const VarDecl *getNRVOCandidate() const { return NRVOCandidate; }
- void setNRVOCandidate(const VarDecl *Var) { NRVOCandidate = Var; }
+ const VarDecl *getNRVOCandidate() const {
+return hasNRVOCandidate() ? *getTrailingObjects()
+ : nullptr;
+ }
- SourceLocation getBeginLoc() const { return getReturnLoc(); }
+ /// Set the variable that might be used for the named return value
+ /// optimization. The return statement must have storage for it,
+ /// which is the case if and only if hasNRVOCandidate() is true.
+ void setNRVOCandidate(const VarDecl *Var) {
+assert(hasNRVOCandidate() &&
+ "This return statement has no storage for an NRVO candidate!");
+*getTrailingObjects() = Var;
+ }
- SourceLocation getEndLoc() const {
+ SourceLocation getReturnLoc() const { return ReturnStmtBits.RetLoc; }
+ void setReturnLoc(SourceLocation L) { ReturnStmtBits.RetLoc = L; }
+
+ SourceLocation getBeginLoc() const { return getReturnLoc(); }
+ SourceLocation getEndLoc() const LLVM_READONLY {
return RetExpr ? RetExpr->getEndLoc() : getReturnLoc();
}
@@ -2042,7 +2072,8 @@
// Iterators
child_range children() {
-if (RetExpr) return child_range(&RetExpr, &RetExpr+1);
+if (RetExpr)
+ return child_range(&RetExpr, &RetExpr + 1);
return chi
[PATCH] D53716: [AST] Only store data for the NRVO candidate in ReturnStmt if needed.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC345605: [AST] Only store data for the NRVO candidate in
ReturnStmt if needed (authored by brunoricci, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D53716?vs=171131&id=171696#toc
Repository:
rC Clang
https://reviews.llvm.org/D53716
Files:
include/clang/AST/Stmt.h
lib/AST/ASTImporter.cpp
lib/AST/Stmt.cpp
lib/Analysis/BodyFarm.cpp
lib/CodeGen/CGObjC.cpp
lib/Sema/SemaStmt.cpp
lib/Serialization/ASTReaderStmt.cpp
lib/Serialization/ASTWriterStmt.cpp
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -5957,8 +5957,8 @@
const VarDecl *ToNRVOCandidate;
std::tie(ToReturnLoc, ToRetValue, ToNRVOCandidate) = *Imp;
- return new (Importer.getToContext()) ReturnStmt(
- ToReturnLoc, ToRetValue, ToNRVOCandidate);
+ return ReturnStmt::Create(Importer.getToContext(), ToReturnLoc, ToRetValue,
+ToNRVOCandidate);
}
ExpectedStmt ASTNodeImporter::VisitCXXCatchStmt(CXXCatchStmt *S) {
Index: lib/AST/Stmt.cpp
===
--- lib/AST/Stmt.cpp
+++ lib/AST/Stmt.cpp
@@ -1042,11 +1042,33 @@
}
// ReturnStmt
-const Expr* ReturnStmt::getRetValue() const {
- return cast_or_null(RetExpr);
-}
-Expr* ReturnStmt::getRetValue() {
- return cast_or_null(RetExpr);
+ReturnStmt::ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate)
+: Stmt(ReturnStmtClass), RetExpr(E) {
+ bool HasNRVOCandidate = NRVOCandidate != nullptr;
+ ReturnStmtBits.HasNRVOCandidate = HasNRVOCandidate;
+ if (HasNRVOCandidate)
+setNRVOCandidate(NRVOCandidate);
+ setReturnLoc(RL);
+}
+
+ReturnStmt::ReturnStmt(EmptyShell Empty, bool HasNRVOCandidate)
+: Stmt(ReturnStmtClass, Empty) {
+ ReturnStmtBits.HasNRVOCandidate = HasNRVOCandidate;
+}
+
+ReturnStmt *ReturnStmt::Create(const ASTContext &Ctx, SourceLocation RL,
+ Expr *E, const VarDecl *NRVOCandidate) {
+ bool HasNRVOCandidate = NRVOCandidate != nullptr;
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(HasNRVOCandidate),
+ alignof(ReturnStmt));
+ return new (Mem) ReturnStmt(RL, E, NRVOCandidate);
+}
+
+ReturnStmt *ReturnStmt::CreateEmpty(const ASTContext &Ctx,
+bool HasNRVOCandidate) {
+ void *Mem = Ctx.Allocate(totalSizeToAlloc(HasNRVOCandidate),
+ alignof(ReturnStmt));
+ return new (Mem) ReturnStmt(EmptyShell(), HasNRVOCandidate);
}
// CaseStmt
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -3226,7 +3226,8 @@
return StmtError();
RetValExp = ER.get();
}
-return new (Context) ReturnStmt(ReturnLoc, RetValExp, nullptr);
+return ReturnStmt::Create(Context, ReturnLoc, RetValExp,
+ /* NRVOCandidate=*/nullptr);
}
if (HasDeducedReturnType) {
@@ -3352,8 +3353,8 @@
return StmtError();
RetValExp = ER.get();
}
- ReturnStmt *Result = new (Context) ReturnStmt(ReturnLoc, RetValExp,
-NRVOCandidate);
+ auto *Result =
+ ReturnStmt::Create(Context, ReturnLoc, RetValExp, NRVOCandidate);
// If we need to check for the named return value optimization,
// or if we need to infer the return type,
@@ -3582,7 +3583,8 @@
return StmtError();
RetValExp = ER.get();
}
-return new (Context) ReturnStmt(ReturnLoc, RetValExp, nullptr);
+return ReturnStmt::Create(Context, ReturnLoc, RetValExp,
+ /* NRVOCandidate=*/nullptr);
}
// FIXME: Add a flag to the ScopeInfo to indicate whether we're performing
@@ -3677,7 +3679,8 @@
}
}
-Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, nullptr);
+Result = ReturnStmt::Create(Context, ReturnLoc, RetValExp,
+/* NRVOCandidate=*/nullptr);
} else if (!RetValExp && !HasDependentReturnType) {
FunctionDecl *FD = getCurFunctionDecl();
@@ -3699,7 +3702,8 @@
else
Diag(ReturnLoc, DiagID) << getCurMethodDecl()->getDeclName() << 1/*meth*/;
-Result = new (Context) ReturnStmt(ReturnLoc);
+Result = ReturnStmt::Create(Context, ReturnLoc, /* RetExpr=*/nullptr,
+/* NRVOCandidate=*/nullptr);
} else {
assert(RetValExp || HasDependentReturnType);
const VarDecl *NRVOCandidate = nullptr;
@@ -3752,7 +3756,7 @@
return StmtError();
RetValExp = ER.get();
}
-Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, NRVOCandidate);
+Result = ReturnStmt::Create(Context, ReturnLoc, RetValExp, NRVOCandidate);
}
// If we need to check for the named return value optimization, save the
I
[PATCH] D53025: [clang-tidy] implement new check for const return types.
ymandel updated this revision to Diff 171700.
ymandel added a comment.
Reworded diagnostic message and corresponding documentation.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53025
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ConstValueReturnCheck.cpp
clang-tidy/readability/ConstValueReturnCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/utils/LexerUtils.cpp
clang-tidy/utils/LexerUtils.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-const-value-return.rst
test/clang-tidy/readability-const-value-return.cpp
Index: test/clang-tidy/readability-const-value-return.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-const-value-return.cpp
@@ -0,0 +1,227 @@
+// RUN: %check_clang_tidy %s readability-const-value-return %t -- -- -isystem
+
+// p# = positive test
+// n# = negative test
+
+#include
+
+const int p1() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qualified, which may reduce code readability without improving const correctness
+// CHECK-FIXES: int p1() {
+ return 1;
+}
+
+const int p15();
+// CHECK-FIXES: int p15();
+
+template
+const int p31(T v) { return 2; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+// CHECK-FIXES: int p31(T v) { return 2; }
+
+// We detect const-ness even without instantiating T.
+template
+const T p32(T t) { return t; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const T' is 'const'-qual
+// CHECK-FIXES: T p32(T t) { return t; }
+
+// However, if the return type is itself a template instantiation, Clang does
+// not consider it const-qualified without knowing `T`.
+template
+typename std::add_const::type n15(T v) { return v; }
+
+template
+class Klazz {
+public:
+ Klazz(A) {}
+};
+
+class Clazz {
+ public:
+ Clazz *const p2() {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'Clazz *const' is 'co
+// CHECK-FIXES: Clazz *p2() {
+return this;
+ }
+
+ Clazz *const p3();
+ // CHECK-FIXES: Clazz *p3();
+
+ const int p4() const {
+// CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const
+// CHECK-FIXES: int p4() const {
+return 4;
+ }
+
+ const Klazz* const p5() const;
+ // CHECK-FIXES: const Klazz* p5() const;
+
+ const Clazz operator++(int x) { // p12
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz' is 'const
+ // CHECK-FIXES: Clazz operator++(int x) {
+ }
+
+ struct Strukt {
+int i;
+ };
+
+ const Strukt p6() {}
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i
+ // CHECK-FIXES: Strukt p6() {}
+
+ // No warning is emitted here, because this is only the declaration. The
+ // warning will be associated with the definition, below.
+ const Strukt* const p7();
+ // CHECK-FIXES: const Strukt* p7();
+
+ // const-qualifier is the first `const` token, but not the first token.
+ static const int p8() {}
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-
+ // CHECK-FIXES: static int p8() {}
+
+ static const Strukt p9() {}
+ // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i
+ // CHECK-FIXES: static Strukt p9() {}
+
+ int n0() const { return 0; }
+ const Klazz& n11(const Klazz) const;
+};
+
+Clazz *const Clazz::p3() {
+ // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'Clazz *const' is 'cons
+ // CHECK-FIXES: Clazz *Clazz::p3() {
+ return this;
+}
+
+const Klazz* const Clazz::p5() const {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* Clazz::p5() const {}
+
+const Clazz::Strukt* const Clazz::p7() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz::Strukt *con
+// CHECK-FIXES: const Clazz::Strukt* Clazz::p7() {}
+
+Clazz *const p10();
+// CHECK-FIXES: Clazz *p10();
+
+Clazz *const p10() {
+ // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'Clazz *const' is 'cons
+ // CHECK-FIXES: Clazz *p10() {
+ return new Clazz();
+}
+
+const Clazz bar;
+const Clazz *const p11() {
+ // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz *const' is
+ // CHECK-FIXES: const Clazz *p11() {
+ return &bar;
+}
+
+const Klazz p12() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz'
+// CHECK-FIXES: Klazz p12() {}
+
+const Klazz* const p13() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* p13() {}
+
+// re-declaration of p15.
+const int p15();
+// CHECK-FIXES: int p15();
+
+const int p15() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning:
+// CHECK-FIXES: int p15() {
+ return 0;
+}
+
+// Exercise the lexer.
+
+const /* comment */ /* another comment*/ int p16() { return 0; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning:
+// CHECK-FIXES: /* comment */ /* anoth
[PATCH] D53025: [clang-tidy] implement new check for const return types.
ymandel marked 3 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<< Def->getReturnType(); aaron.ballman wrote: > ymandel wrote: > > aaron.ballman wrote: > > > ymandel wrote: > > > > aaron.ballman wrote: > > > > > ymandel wrote: > > > > > > aaron.ballman wrote: > > > > > > > It seems strange to me that this is in the readability module but > > > > > > > the diagnostic here is about compiler optimizations. Should this > > > > > > > be in the performance module instead? Or should this diagnostic > > > > > > > be reworded about the readability concerns? > > > > > > Good point. The readability angle is that the const clutters the > > > > > > code to no benefit. That is, even if it was performance neutral, > > > > > > I'd still want to discourage this practice. But, it does seem like > > > > > > the primary impact is performance. > > > > > > > > > > > > I'm fine either way -- moving it to performance or emphasizing the > > > > > > clutter of the unhelpful "const". I'm inclined to moving it, but > > > > > > don't have good sense of how strict these hierarchies are. What do > > > > > > you think? > > > > > I'm not sold that `const`-qualified return types always pessimize > > > > > optimizations. However, I'm also not sold that it's *always* a bad > > > > > idea to have a top-level cv-qualifier on a return type either (for > > > > > instance, this is one way to prevent callers from modifying a temp > > > > > returned by a function). How about leaving this in readability and > > > > > changing the diagnostic into: "top-level 'const' qualifier on a > > > > > return type may reduce code readability with limited benefit" or > > > > > something equally weasely? > > > > I talked this over with other google folks and seems like the consensus > > > > is: > > > > > > > > 1. The readability benefits may be controversial. Some folks might > > > > not view `const` as clutter and there are some corner cases where the > > > > qualifier may prevent something harmful. > > > > 2. The performance implication is real, if not guaranteed to be a > > > > problem. > > > > > > > > Based on this, seems best to move it to performance, but water down the > > > > performance claims. That keeps the focus to an issue we can assume > > > > nearly everyone agrees on. > > > I'm not convinced the performance implications are real compared to how > > > the check is currently implemented. I know there are performance concerns > > > when you return a const value of class type because it pessimizes the > > > ability to use move constructors, but that's a very specific performance > > > concern that I don't believe is present in general. For instance, I don't > > > know of any performance concerns regarding `const int f();` as a > > > declaration. I'd be fine moving this to the performance module, but I > > > think the check would need to be tightened to only focus on actual > > > performance concerns. > > > > > > FWIW, I do agree that the readability benefits may be controversial, but > > > I kind of thought that was the point to the check and as such, it's a > > > very reasonable check. Almost everything in readability is subjective to > > > some degree and our metric has always been "if you agree with a style > > > check, don't enable it". > > > > > > It's up to you how you want to proceed, but I see two ways forward: 1) > > > keep this as a readability check that broadly finds top-level const > > > qualifiers and removes them, 2) move this to the performance module and > > > rework the check to focus on only the scenarios that have concrete > > > performance impact. > > > It's up to you how you want to proceed, but I see two ways forward: 1) > > > keep this as a readability check that broadly finds top-level const > > > qualifiers and removes them, 2) move this to the performance module and > > > rework the check to focus on only the scenarios that have concrete > > > performance impact. > > > > Aaron, thank you for the detailed response. I'm inclined to agree with you > > that this belongs in readabilty and I spoke with sbenza who feels the same. > > The high-level point is that the `const` is noise in most cases. > > > > You suggested above a warning along the lines of: > > "top-level 'const' qualifier on a return type may reduce code readability > > with limited benefit" > > > > I like this, but think it should be a little stronger. Perhaps: > > "top-level 'const' qualifier on a return type may reduce code readability > > while rarely having an effect" > > > > I also propose updating the explanation in the documentation file from: > > > > Such use of ``const`` is superfluous, and > > prevents valuable compiler opti
[PATCH] D53654: [clang] Improve ctor initializer completions.
kadircet updated this revision to Diff 171703.
kadircet marked 8 inline comments as done.
kadircet added a comment.
- Address comments.
Repository:
rC Clang
https://reviews.llvm.org/D53654
Files:
lib/Sema/SemaCodeComplete.cpp
test/CodeCompletion/ctor-initializer.cpp
test/Index/complete-ctor-inits.cpp
test/Index/complete-cxx-inline-methods.cpp
Index: test/Index/complete-cxx-inline-methods.cpp
===
--- test/Index/complete-cxx-inline-methods.cpp
+++ test/Index/complete-cxx-inline-methods.cpp
@@ -21,6 +21,13 @@
int value;
MyCls *object;
};
+
+template
+class X {};
+
+class Y : public X {
+ Y() : X() {}
+};
}
// RUN: c-index-test -code-completion-at=%s:4:9 -std=c++98 %s | FileCheck %s
@@ -35,10 +42,12 @@
// CHECK-NEXT: Container Kind: StructDecl
// RUN: c-index-test -code-completion-at=%s:18:41 %s | FileCheck -check-prefix=CHECK-CTOR-INIT %s
-// CHECK-CTOR-INIT: NotImplemented:{TypedText MyCls}{LeftParen (}{Placeholder args}{RightParen )} (7)
-// CHECK-CTOR-INIT: MemberRef:{TypedText object}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CTOR-INIT: MemberRef:{TypedText value}{LeftParen (}{Placeholder args}{RightParen )} (35)
+// CHECK-CTOR-INIT: ClassDecl:{TypedText MyCls}{LeftParen (}{Placeholder MyCls}{RightParen )} (7)
+// CHECK-CTOR-INIT: FieldDecl:{TypedText object}{LeftParen (}{Placeholder MyCls *}{RightParen )} (35)
+// CHECK-CTOR-INIT: FieldDecl:{TypedText value}{LeftParen (}{Placeholder int}{RightParen )} (35)
// RUN: c-index-test -code-completion-at=%s:18:55 %s | FileCheck -check-prefix=CHECK-CTOR-INIT-2 %s
-// CHECK-CTOR-INIT-2-NOT: NotImplemented:{TypedText MyCls}{LeftParen (}{Placeholder args}{RightParen )}
-// CHECK-CTOR-INIT-2: MemberRef:{TypedText object}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CTOR-INIT-2: MemberRef:{TypedText value}{LeftParen (}{Placeholder args}{RightParen )} (7)
+// CHECK-CTOR-INIT-2-NOT: ClassDecl:{TypedText MyCls}{LeftParen (}{Placeholder MyCls}{RightParen )} (7)
+// CHECK-CTOR-INIT-2: FieldDecl:{TypedText object}{LeftParen (}{Placeholder MyCls *}{RightParen )} (35)
+// CHECK-CTOR-INIT-2: FieldDecl:{TypedText value}{LeftParen (}{Placeholder int}{RightParen )} (7)
+// RUN: c-index-test -code-completion-at=%s:29:9 %s | FileCheck -check-prefix=CHECK-CTOR-INIT-3 %s
+// CHECK-CTOR-INIT-3: ClassDecl:{TypedText X}{LeftParen (}{Placeholder X}{RightParen )} (7)
Index: test/Index/complete-ctor-inits.cpp
===
--- test/Index/complete-ctor-inits.cpp
+++ test/Index/complete-ctor-inits.cpp
@@ -30,27 +30,33 @@
};
// RUN: c-index-test -code-completion-at=%s:18:10 %s | FileCheck -check-prefix=CHECK-CC1 %s
-// CHECK-CC1: MemberRef:{TypedText a}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC1: MemberRef:{TypedText b}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC1: MemberRef:{TypedText c}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC1: NotImplemented:{TypedText Virt}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC1: NotImplemented:{TypedText X}{LeftParen (}{Placeholder args}{RightParen )} (7)
-// CHECK-CC1: NotImplemented:{TypedText Y}{LeftParen (}{Placeholder args}{RightParen )} (35)
+// CHECK-CC1: FieldDecl:{TypedText a}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC1: FieldDecl:{TypedText b}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC1: FieldDecl:{TypedText c}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC1: CXXConstructor:{TypedText Virt}{LeftParen (}{Placeholder const Virt &}{RightParen )} (35)
+// CHECK-CC1: CXXConstructor:{TypedText Virt}{LeftParen (}{Placeholder Virt &&}{RightParen )} (35)
+// CHECK-CC1: CXXConstructor:{TypedText X}{LeftParen (}{Placeholder int}{RightParen )} (7)
+// CHECK-CC1: CXXConstructor:{TypedText Y}{LeftParen (}{Placeholder const Y &}{RightParen )} (35)
+// CHECK-CC1: CXXConstructor:{TypedText Y}{LeftParen (}{Placeholder Y &&}{RightParen )} (35)
// RUN: c-index-test -code-completion-at=%s:18:23 %s | FileCheck -check-prefix=CHECK-CC2 %s
-// CHECK-CC2: MemberRef:{TypedText a}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC2: MemberRef:{TypedText b}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC2: MemberRef:{TypedText c}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC2: NotImplemented:{TypedText Virt}{LeftParen (}{Placeholder args}{RightParen )} (35)
-// CHECK-CC2: NotImplemented:{TypedText Y}{LeftParen (}{Placeholder args}{RightParen )} (7)
+// CHECK-CC2: FieldDecl:{TypedText a}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC2: FieldDecl:{TypedText b}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC2: FieldDecl:{TypedText c}{LeftParen (}{Placeholder int}{RightParen )} (35)
+// CHECK-CC2: CXXConstructor:{TypedText Virt}{LeftParen (}{Placeholder const Virt &}{RightParen )} (35)
+// CHECK
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet added a comment. So I ran `llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py` The bare version produces `65664` unique findings. The version restricted to `cppcoreguidelines-narrowing-conversions` produces `4323` unique findings. That's about `+7%` of findings. I can post some random ones is you're interested. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53654: [clang] Improve ctor initializer completions.
kadircet added inline comments.
Comment at: lib/Sema/SemaCodeComplete.cpp:821
+
+DeclContext::lookup_result getConstructorResults(ASTContext &Context,
+ const CXXRecordDecl *Record,
ilya-biryukov wrote:
> There's a function that does the same thing and also does some extra lifting
> to make sure implicit members are properly generated -
> `Sema::LookupConstructors`. Maybe use it instead?
As talked offline we don't want this behavior.
Comment at: lib/Sema/SemaCodeComplete.cpp:831
+ // template args and one parameter with type's name.
+ /*
+ if (Ctors.begin() == Ctors.end()) {
ilya-biryukov wrote:
> Remove this code instead of commenting it out?
Ah, sorry for the mess :(
Comment at: lib/Sema/SemaCodeComplete.cpp:5136
+ auto AddDefaultCtorInit = [&](const char *TypedName,
+const char *TypeName,
+const NamedDecl* ND) {
ilya-biryukov wrote:
> Maybe use StringRef?
Don't think this looks any better, since we need to use `.data()` at chunk
additions than.
Even if we try to push allocations all the way down until we add those chunks,
we still get bad looking code since types are returned as `std::string` and we
need to make a copy in the stack to be able to pass it as `StringRef`.
Repository:
rC Clang
https://reviews.llvm.org/D53654
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53870: [clang-cl] Put dllexport attrs on static locals also in template instantiations (PR39496)
hans created this revision.
hans added reviewers: rnk, thakis, takuto.ikuta.
In the course of https://reviews.llvm.org/D51340, @takuto.ikuta discovered that
Clang fails to put dllexport/import attributes on static locals during template
instantiation.
For regular functions, this happens in Sema::FinalizeDeclaration(), however for
template instantiations we need to do something in or around
TemplateDeclInstantiator::VisitVarDecl(). This patch does that, and extracts
the code to a utility function.
Please take a look!
https://reviews.llvm.org/D53870
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaDecl.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/dllimport.cpp
Index: test/CodeGenCXX/dllimport.cpp
===
--- test/CodeGenCXX/dllimport.cpp
+++ test/CodeGenCXX/dllimport.cpp
@@ -996,3 +996,16 @@
USEMEMFUNC(ExplicitInstantiationDeclTemplateBase2, func)
// M32-DAG: declare dllimport x86_thiscallcc void @"?func@?$ExplicitInstantiationDeclTemplateBase2@H@@QAEXXZ"
// G32-DAG: define weak_odr dso_local x86_thiscallcc void @_ZN38ExplicitInstantiationDeclTemplateBase2IiE4funcEv
+
+namespace pr39496 {
+// Make sure dll attribute are inherited by static locals also in template
+// specializations.
+template struct __declspec(dllimport) S { int foo() { static int x; return x++; } };
+int foo() { S s; return s.foo(); }
+// MO1-DAG: @"?x@?{{1|2}}??foo@?$S@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = available_externally dllimport global i32 0, align 4
+
+template struct T { int foo() { static int x; return x++; } };
+extern template struct __declspec(dllimport) T;
+int bar() { T t; return t.foo(); }
+// MO1-DAG: @"?x@?{{1|2}}??foo@?$T@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = available_externally dllimport global i32 0, align 4
+}
Index: test/CodeGenCXX/dllexport.cpp
===
--- test/CodeGenCXX/dllexport.cpp
+++ test/CodeGenCXX/dllexport.cpp
@@ -1012,6 +1012,18 @@
// M32-DAG: define weak_odr dso_local dllexport x86_thiscallcc %"struct.LayerTreeImpl::ElementLayers"* @"??0ElementLayers@LayerTreeImpl@@QAE@XZ"
// M64-DAG: define weak_odr dso_local dllexport %"struct.LayerTreeImpl::ElementLayers"* @"??0ElementLayers@LayerTreeImpl@@QEAA@XZ"
+namespace pr39496 {
+// Make sure dll attribute are inherited by static locals also in template
+// specializations.
+template struct __declspec(dllexport) S { int foo() { static int x; return x++; } };
+int foo() { S s; return s.foo(); }
+// MSC-DAG: @"?x@?{{1|2}}??foo@?$S@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+
+template struct T { int foo() { static int x; return x++; } };
+template struct __declspec(dllexport) T;
+// MSC-DAG: @"?x@?{{1|2}}??foo@?$T@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+}
+
class __declspec(dllexport) ACE_Shared_Object {
public:
virtual ~ACE_Shared_Object();
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -728,6 +728,9 @@
D->getLocation(), D->getIdentifier(), DI->getType(),
DI, D->getStorageClass());
+ if (Var->isStaticLocal())
+SemaRef.CheckStaticLocalForDllExport(Var);
+
// In ARC, infer 'retaining' for variables of retainable type.
if (SemaRef.getLangOpts().ObjCAutoRefCount &&
SemaRef.inferObjCARCLifetime(Var))
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11924,6 +11924,23 @@
return false;
}
+/// Check if VD needs to be dllexport/dllimport due to being in a
+/// dllexport/import function.
+void Sema::CheckStaticLocalForDllExport(VarDecl *VD) {
+ assert(VD->isStaticLocal());
+
+ auto *FD = dyn_cast_or_null(VD->getParentFunctionOrMethod());
+ if (!FD)
+return;
+
+ // Static locals inherit dll attributes from their function.
+ if (Attr *A = getDLLAttr(FD)) {
+auto *NewAttr = cast(A->clone(getASTContext()));
+NewAttr->setInherited(true);
+VD->addAttr(NewAttr);
+ }
+}
+
/// FinalizeDeclaration - called by ParseDeclarationAfterDeclarator to perform
/// any semantic actions necessary after any initializer has been attached.
void Sema::FinalizeDeclaration(Decl *ThisDecl) {
@@ -11977,14 +11994,9 @@
}
if (VD->isStaticLocal()) {
-if (FunctionDecl *FD =
-dyn_cast_or_null(VD->getParentFunctionOrMethod())) {
- // Static locals inherit dll attributes from their function.
- if (Attr *A = getDLLAttr(FD)) {
-auto *NewAttr = cast(A->clone(getASTContext()));
-NewAttr->setInherited(true);
-VD->addAttr(NewAttr);
- }
+CheckStaticLocalForDllExport(VD);
+
+if (dyn_cast_or_nul
[PATCH] D53648: [clangd] Only log ignored diagnostics with -log=verbose.
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53648: [clangd] Only log ignored diagnostics with -log=verbose.
ilya-biryukov added a comment. Sorry for the delay, it was open in my browser for days... Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53814: Allow the analyzer to output to a SARIF file
aaron.ballman updated this revision to Diff 171712.
aaron.ballman added a comment.
Switched to using diff for testing.
https://reviews.llvm.org/D53814
Files:
Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
Analysis/diagnostics/sarif-diagnostics-taint-test.c
StaticAnalyzer/Core/CMakeLists.txt
StaticAnalyzer/Core/SarifDiagnostics.cpp
clang/StaticAnalyzer/Core/Analyses.def
clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
Index: Analysis/diagnostics/sarif-diagnostics-taint-test.c
===
--- /dev/null
+++ Analysis/diagnostics/sarif-diagnostics-taint-test.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.security.taint,debug.TaintTest %s -verify -analyzer-output=sarif -o - | diff -u1 -w -I ".*file:.*sarif-diagnostics-taint-test.c" -I "clang version" -I "2\.0\.0\-beta\." - %S/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
+#include "../Inputs/system-header-simulator.h"
+
+int atoi(const char *nptr);
+
+void f(void) {
+ char s[80];
+ scanf("%s", s);
+ int d = atoi(s); // expected-warning {{tainted}}
+}
+
+int main(void) {
+ f();
+ return 0;
+}
Index: Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
===
--- /dev/null
+++ Analysis/diagnostics/Inputs/expected-sarif/sarif-diagnostics-taint-test.c.sarif
@@ -0,0 +1,99 @@
+{
+ "$schema": "http://json.schemastore.org/sarif-2.0.0";,
+ "runs": [
+{
+ "files": {
+"file:sarif-diagnostics-taint-test.c": {
+ "fileLocation": {
+"uri": "file:sarif-diagnostics-taint-test.c"
+ },
+ "length": 500,
+ "mimeType": "text/plain",
+ "roles": [
+"resultFile"
+ ]
+}
+ },
+ "results": [
+{
+ "codeFlows": [
+{
+ "threadFlows": [
+{
+ "locations": [
+{
+ "importance": "essential",
+ "location": {
+"message": {
+ "text": "Calling 'f'"
+},
+"physicalLocation": {
+ "fileLocation": {
+"uri": "file:sarif-diagnostics-taint-test.c"
+ },
+ "region": {
+"endColumn": 5,
+"endLine": 13,
+"startColumn": 3,
+"startLine": 13
+ }
+}
+ },
+ "step": 1
+},
+{
+ "importance": "essential",
+ "location": {
+"message": {
+ "text": "tainted"
+},
+"physicalLocation": {
+ "fileLocation": {
+"uri": "file:sarif-diagnostics-taint-test.c"
+ },
+ "region": {
+"endColumn": 17,
+"endLine": 9,
+"startColumn": 11,
+"startLine": 9
+ }
+}
+ },
+ "step": 2
+}
+ ]
+}
+ ]
+}
+ ],
+ "locations": [
+{
+ "physicalLocation": {
+"fileLocation": {
+ "uri": "file:sarif-diagnostics-taint-test.c"
+},
+"region": {
+ "endColumn": 17,
+ "endLine": 9,
+ "startColumn": 11,
+ "startLine": 9
+}
+ }
+}
+ ],
+ "message": {
+"text": "tainted"
+ },
+ "ruleId": "debug.TaintTest"
+}
+ ],
+ "tool": {
+"fullName": "clang static analyzer",
+"language": "en-US",
+"name": "clang",
+"version": "clang version 8.0.0 (https://github.com/llvm-project/clang.git a5ccb257a7a70928ede717a7c282f5fc8cbed310) (https://github.com/llvm-mirror/llvm.git 73cebd79c512f7129eca16b0f3a7abd21d2881e8)"
+ }
+}
+ ],
+ "version": "2.0.0-beta.2018-09-26"
+}
Index: StaticAnalyzer/Core/SarifDiagnostics.cpp
===
--- /dev/null
+++ StaticAnalyzer/Core/SarifDiagnostics.cpp
@@ -0,0 +1,270 @@
+//===--- SarifDiagnostics.cpp - Sarif Diagnostics for Paths -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under t
[PATCH] D53871: [OpenCL] Allow clk_event_t comparisons
svenvh created this revision.
svenvh added a reviewer: mantognini.
Herald added subscribers: cfe-commits, Anastasia, yaxunl.
Also rename `invalid-clk-events-cl2.0.cl` to `clk_event_t.cl` and
repurpose it to include both positive and negative clk_event_t tests.
Repository:
rC Clang
https://reviews.llvm.org/D53871
Files:
lib/Sema/SemaExpr.cpp
test/SemaOpenCL/clk_event_t.cl
test/SemaOpenCL/invalid-clk-events-cl2.0.cl
Index: test/SemaOpenCL/clk_event_t.cl
===
--- test/SemaOpenCL/clk_event_t.cl
+++ test/SemaOpenCL/clk_event_t.cl
@@ -1,3 +1,24 @@
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
+#define CLK_NULL_EVENT (__builtin_astype(((void*)(__SIZE_MAX__)), clk_event_t))
+
global clk_event_t ce; // expected-error {{the '__global clk_event_t' type
cannot be used to declare a program scope variable}}
+
+int clk_event_tests() {
+ event_t e;
+ clk_event_t ce1;
+ clk_event_t ce2;
+
+ if (e == ce1) { // expected-error {{invalid operands to binary expression
('event_t' and 'clk_event_t')}}
+return 9;
+ }
+
+ if (ce1 != ce2) {
+return 1;
+ }
+ else if (ce1 == CLK_NULL_EVENT || ce2 != CLK_NULL_EVENT) {
+return 0;
+ }
+
+ return 2;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10497,6 +10497,10 @@
}
if (getLangOpts().OpenCLVersion >= 200) {
+if (LHSType->isClkEventT() && RHSType->isClkEventT()) {
+ return computeResultTy();
+}
+
if (LHSType->isQueueT() && RHSType->isQueueT()) {
return computeResultTy();
}
Index: test/SemaOpenCL/clk_event_t.cl
===
--- test/SemaOpenCL/clk_event_t.cl
+++ test/SemaOpenCL/clk_event_t.cl
@@ -1,3 +1,24 @@
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0
+#define CLK_NULL_EVENT (__builtin_astype(((void*)(__SIZE_MAX__)), clk_event_t))
+
global clk_event_t ce; // expected-error {{the '__global clk_event_t' type cannot be used to declare a program scope variable}}
+
+int clk_event_tests() {
+ event_t e;
+ clk_event_t ce1;
+ clk_event_t ce2;
+
+ if (e == ce1) { // expected-error {{invalid operands to binary expression ('event_t' and 'clk_event_t')}}
+return 9;
+ }
+
+ if (ce1 != ce2) {
+return 1;
+ }
+ else if (ce1 == CLK_NULL_EVENT || ce2 != CLK_NULL_EVENT) {
+return 0;
+ }
+
+ return 2;
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -10497,6 +10497,10 @@
}
if (getLangOpts().OpenCLVersion >= 200) {
+if (LHSType->isClkEventT() && RHSType->isClkEventT()) {
+ return computeResultTy();
+}
+
if (LHSType->isQueueT() && RHSType->isQueueT()) {
return computeResultTy();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
lebedev.ri added a comment. In https://reviews.llvm.org/D53817#1280405, @JonasToth wrote: > LGTM Thank you for the review! > @aaron.ballman do you have more comments? If there are no more comments, i'll land this in a few (~3) hours. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53814: Allow the analyzer to output to a SARIF file
aaron.ballman added inline comments.
Comment at: Analysis/diagnostics/sarif-diagnostics-taint-test.c:18
+// Test the basics for sanity.
+// CHECK: sarifLog['version'].startswith("2.0.0")
+// CHECK: sarifLog['runs'][0]['tool']['fullName'] == "clang static analyzer"
aaron.ballman wrote:
> george.karpenkov wrote:
> > aaron.ballman wrote:
> > > george.karpenkov wrote:
> > > > Would it make more sense to just use `diff` + json pretty-formatter to
> > > > write a test?
> > > > With this test I can't even quite figure out how the output should look
> > > > like.
> > > I'm not super comfortable with that approach, but perhaps I'm thinking of
> > > something different than what you're proposing. The reason I went with
> > > this approach is because diff would be fragile (depends heavily on field
> > > ordering, which the JSON support library doesn't give control over) and
> > > the physical layout of the file isn't what needs to be tested anyway.
> > > SARIF has a fair amount of optional data that can be provided as well, so
> > > using a purely textual diff worried me that exporting additional optional
> > > data in the future would require extensive unrelated changes to all SARIF
> > > diffs in the test directory.
> > >
> > > The goal for this test was to demonstrate that we can validate that the
> > > interesting bits of information are present in the output without
> > > worrying about the details.
> > >
> > > Also, the python approach allows us to express relationships between data
> > > items more easily than a textual diff tool would. I've not used that
> > > here, but I could imagine a test where we want to check that each code
> > > location has a corresponding file entry in another list.
> > Using a sample file + diff would have an advantage of being easier to read
> > (just glance at the "Inputs/blah.serif" to see a sample output), and
> > consistent with how we already do checking for plists.
> >
> > > depends heavily on field ordering
> >
> > Is it an issue in practice though? I would assume that JSON support library
> > would not switch field ordering too often (and even if it does, we can have
> > a python wrapper testing that)
> >
> > > SARIF has a fair amount of optional data
> >
> > Would diff `--ignore-matching-lines` be enough for those?
> Diff testing was what I originally tried and I abandoned it because it was
> not viable. The optional data cannot be handled by ignoring matching lines
> because the lines won't match from system to system. For instance, there are
> absolute URIs to files included in the output, clang git hashes and version
> numbers that will change from revision to revision, etc.
>
> What you see as an advantage, I see as more of a distraction -- the actual
> layout of the information in the file isn't that important so long as it
> validates as valid SARIF (unfortunately, there are no cross-platform command
> line tools to validate SARIF yet). What is important are just a few pieces of
> the content information (where are the diagnostics, what source ranges and
> paths are involved, etc) compared to the overall whole.
>
> I can give diff testing another shot, but I was unable to find a way to make
> `-I` work so that I could ignore everything that needs to be ignored due to
> natural variance. Do you have ideas there?
>
> To give you an idea of the ultimate form of the output:
> ```
> {
> "$schema": "http://json.schemastore.org/sarif-2.0.0";,
> "runs": [
> {
> "files": {
> "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c": {
> "fileLocation": {
> "uri": "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
> },
> "length": 3850,
> "mimeType": "text/plain",
> "roles": [
> "resultFile"
> ]
> }
> },
> "results": [
> {
> "codeFlows": [
> {
> "threadFlows": [
> {
> "locations": [
> {
> "importance": "essential",
> "location": {
> "message": {
> "text": "Calling 'f'"
> },
> "physicalLocation": {
> "fileLocation": {
> "uri":
> "file:///C:/Users/aballman.GRAMMATECH/Desktop/test.c"
> },
> "region": {
> "endColumn": 5,
> "endLine": 119,
> "startColumn": 3,
> "startLine": 119
> }
> }
> },
> "step": 1
> },
> {
> "importance": "essential",
> "location": {
>
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
JonasToth requested changes to this revision. JonasToth added a comment. This revision now requires changes to proceed. because this now diverged quite a bit i will revert the lgtm for now and take a longer look at the new patch. The numbers you reported sound good. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32 - const auto IsFloatExpr = - expr(hasType(realFloatingPointType()), unless(IsCeilFloorCall)); + const auto IsBuiltinOrEnumType = anyOf(builtinType(), enumType()); This matcher seems no to be used, did I overlook sth? Comment at: docs/ReleaseNotes.rst:77 - New :doc:`abseil-duration-factory-float ` check. that seems to be unrelated Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
JonasToth added inline comments. Comment at: docs/ReleaseNotes.rst:77 - New :doc:`abseil-duration-factory-float ` check. JonasToth wrote: > that seems to be unrelated sry for noise, this was part of the history diffs! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53025: [clang-tidy] implement new check for const return types.
aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<< Def->getReturnType(); ymandel wrote: > aaron.ballman wrote: > > ymandel wrote: > > > aaron.ballman wrote: > > > > ymandel wrote: > > > > > aaron.ballman wrote: > > > > > > ymandel wrote: > > > > > > > aaron.ballman wrote: > > > > > > > > It seems strange to me that this is in the readability module > > > > > > > > but the diagnostic here is about compiler optimizations. Should > > > > > > > > this be in the performance module instead? Or should this > > > > > > > > diagnostic be reworded about the readability concerns? > > > > > > > Good point. The readability angle is that the const clutters the > > > > > > > code to no benefit. That is, even if it was performance neutral, > > > > > > > I'd still want to discourage this practice. But, it does seem > > > > > > > like the primary impact is performance. > > > > > > > > > > > > > > I'm fine either way -- moving it to performance or emphasizing > > > > > > > the clutter of the unhelpful "const". I'm inclined to moving it, > > > > > > > but don't have good sense of how strict these hierarchies are. > > > > > > > What do you think? > > > > > > I'm not sold that `const`-qualified return types always pessimize > > > > > > optimizations. However, I'm also not sold that it's *always* a bad > > > > > > idea to have a top-level cv-qualifier on a return type either (for > > > > > > instance, this is one way to prevent callers from modifying a temp > > > > > > returned by a function). How about leaving this in readability and > > > > > > changing the diagnostic into: "top-level 'const' qualifier on a > > > > > > return type may reduce code readability with limited benefit" or > > > > > > something equally weasely? > > > > > I talked this over with other google folks and seems like the > > > > > consensus is: > > > > > > > > > > 1. The readability benefits may be controversial. Some folks might > > > > > not view `const` as clutter and there are some corner cases where the > > > > > qualifier may prevent something harmful. > > > > > 2. The performance implication is real, if not guaranteed to be a > > > > > problem. > > > > > > > > > > Based on this, seems best to move it to performance, but water down > > > > > the performance claims. That keeps the focus to an issue we can > > > > > assume nearly everyone agrees on. > > > > I'm not convinced the performance implications are real compared to how > > > > the check is currently implemented. I know there are performance > > > > concerns when you return a const value of class type because it > > > > pessimizes the ability to use move constructors, but that's a very > > > > specific performance concern that I don't believe is present in > > > > general. For instance, I don't know of any performance concerns > > > > regarding `const int f();` as a declaration. I'd be fine moving this to > > > > the performance module, but I think the check would need to be > > > > tightened to only focus on actual performance concerns. > > > > > > > > FWIW, I do agree that the readability benefits may be controversial, > > > > but I kind of thought that was the point to the check and as such, it's > > > > a very reasonable check. Almost everything in readability is subjective > > > > to some degree and our metric has always been "if you agree with a > > > > style check, don't enable it". > > > > > > > > It's up to you how you want to proceed, but I see two ways forward: 1) > > > > keep this as a readability check that broadly finds top-level const > > > > qualifiers and removes them, 2) move this to the performance module and > > > > rework the check to focus on only the scenarios that have concrete > > > > performance impact. > > > > It's up to you how you want to proceed, but I see two ways forward: 1) > > > > keep this as a readability check that broadly finds top-level const > > > > qualifiers and removes them, 2) move this to the performance module and > > > > rework the check to focus on only the scenarios that have concrete > > > > performance impact. > > > > > > Aaron, thank you for the detailed response. I'm inclined to agree with > > > you that this belongs in readabilty and I spoke with sbenza who feels the > > > same. The high-level point is that the `const` is noise in most cases. > > > > > > You suggested above a warning along the lines of: > > > "top-level 'const' qualifier on a return type may reduce code > > > readability with limited benefit" > > > > > > I like this, but think it should be a little stronger. Perhaps: > > > "top-level 'const' qualifier on a return type may reduce code > > > readability while rarely having an effect" > > > > > > I also propose upda
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
Szelethus added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:14 #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/APInt.h" +#include "llvm/ADT/APSInt.h" Is `APInt` used anywhere? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r345609 - [OPENMP] Support for mapping of the lambdas in target regions.
Author: abataev
Date: Tue Oct 30 08:50:12 2018
New Revision: 345609
URL: http://llvm.org/viewvc/llvm-project?rev=345609&view=rev
Log:
[OPENMP] Support for mapping of the lambdas in target regions.
Added support for mapping of lambdas in the target regions. It scans all
the captures by reference in the lambda, implicitly maps those variables
in the target region and then later reinstate the addresses of
references in lambda to the correct addresses of the captured|privatized
variables.
Added:
cfe/trunk/test/OpenMP/nvptx_lambda_capturing.cpp
Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.h
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.h
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/Sema/SemaOpenMP.cpp
Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=345609&r1=345608&r2=345609&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Tue Oct 30 08:50:12 2018
@@ -7532,6 +7532,76 @@ public:
}
}
+ /// Emit capture info for lambdas for variables captured by reference.
+ void generateInfoForLambdaCaptures(const ValueDecl *VD, llvm::Value *Arg,
+ MapBaseValuesArrayTy &BasePointers,
+ MapValuesArrayTy &Pointers,
+ MapValuesArrayTy &Sizes,
+ MapFlagsArrayTy &Types) const {
+const auto *RD = VD->getType()
+ .getCanonicalType()
+ .getNonReferenceType()
+ ->getAsCXXRecordDecl();
+if (!RD || !RD->isLambda())
+ return;
+Address VDAddr = Address(Arg, CGF.getContext().getDeclAlign(VD));
+LValue VDLVal = CGF.MakeAddrLValue(
+VDAddr, VD->getType().getCanonicalType().getNonReferenceType());
+llvm::DenseMap Captures;
+FieldDecl *ThisCapture = nullptr;
+RD->getCaptureFields(Captures, ThisCapture);
+if (ThisCapture) {
+ LValue ThisLVal =
+ CGF.EmitLValueForFieldInitialization(VDLVal, ThisCapture);
+ BasePointers.push_back(VDLVal.getPointer());
+ Pointers.push_back(ThisLVal.getPointer());
+ Sizes.push_back(CGF.getTypeSize(CGF.getContext().VoidPtrTy));
+ Types.push_back(OMP_MAP_PTR_AND_OBJ | OMP_MAP_PRIVATE |
+ OMP_MAP_MEMBER_OF | OMP_MAP_IMPLICIT);
+}
+for (const LambdaCapture &LC : RD->captures()) {
+ if (LC.getCaptureKind() != LCK_ByRef)
+continue;
+ const VarDecl *VD = LC.getCapturedVar();
+ auto It = Captures.find(VD);
+ assert(It != Captures.end() && "Found lambda capture without field.");
+ LValue VarLVal = CGF.EmitLValueForFieldInitialization(VDLVal,
It->second);
+ BasePointers.push_back(VDLVal.getPointer());
+ Pointers.push_back(VarLVal.getPointer());
+ Sizes.push_back(CGF.getTypeSize(
+ VD->getType().getCanonicalType().getNonReferenceType()));
+ Types.push_back(OMP_MAP_PTR_AND_OBJ | OMP_MAP_PRIVATE |
+ OMP_MAP_MEMBER_OF | OMP_MAP_IMPLICIT);
+}
+ }
+
+ /// Set correct indices for lambdas captures.
+ void adjustMemberOfForLambdaCaptures(MapBaseValuesArrayTy &BasePointers,
+ MapValuesArrayTy &Pointers,
+ MapFlagsArrayTy &Types) const {
+for (unsigned I = 0, E = Types.size(); I < E; ++I) {
+ // Set correct member_of idx for all implicit lambda captures.
+ if (Types[I] != (OMP_MAP_PTR_AND_OBJ | OMP_MAP_PRIVATE |
+ OMP_MAP_MEMBER_OF | OMP_MAP_IMPLICIT))
+continue;
+ llvm::Value *BasePtr = *BasePointers[I];
+ int TgtIdx = -1;
+ for (unsigned J = I; J > 0; --J) {
+unsigned Idx = J - 1;
+if (Pointers[Idx] != BasePtr)
+ continue;
+TgtIdx = Idx;
+break;
+ }
+ assert(TgtIdx != -1 && "Unable to find parent lambda.");
+ // All other current entries will be MEMBER_OF the combined entry
+ // (except for PTR_AND_OBJ entries which do not have a placeholder value
+ // 0x in the MEMBER_OF field).
+ OpenMPOffloadMappingFlags MemberOfFlag = getMemberOfFlag(TgtIdx);
+ setCorrectMemberOfFlag(Types[I], MemberOfFlag);
+}
+ }
+
/// Generate the base pointers, section pointers, sizes and map types
/// associated to a given capture.
void generateInfoForCapture(const CapturedStmt::Capture *Cap,
@@ -8133,6 +8203,12 @@ void CGOpenMPRuntime::emitTargetCall(Cod
if (CurBasePointers.empty())
MEHandler.generateDefaultMapInfo(*CI, **RI, *CV, CurBasePointers,
CurPointers, CurSizes, CurMapTypes);
+// Ge
[PATCH] D53817: [clang-tidy] cppcoreguidelines-macro-usage: print macro names
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345610: [clang-tidy] cppcoreguidelines-macro-usage: print
macro names (authored by lebedevri, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53817?vs=171683&id=171714#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53817
Files:
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage.cpp
Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -31,64 +31,76 @@
class MacroUsageCallbacks : public PPCallbacks {
public:
- MacroUsageCallbacks(MacroUsageCheck *Check, StringRef RegExp, bool CapsOnly)
- : Check(Check), RegExp(RegExp), CheckCapsOnly(CapsOnly) {}
+ MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM,
+ StringRef RegExp, bool CapsOnly, bool IgnoreCommandLine)
+ : Check(Check), SM(SM), RegExp(RegExp), CheckCapsOnly(CapsOnly),
+IgnoreCommandLineMacros(IgnoreCommandLine) {}
void MacroDefined(const Token &MacroNameTok,
const MacroDirective *MD) override {
if (MD->getMacroInfo()->isUsedForHeaderGuard() ||
MD->getMacroInfo()->getNumTokens() == 0)
return;
+if (IgnoreCommandLineMacros &&
+SM.isWrittenInCommandLineFile(MD->getLocation()))
+ return;
+
StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName();
if (!CheckCapsOnly && !llvm::Regex(RegExp).match(MacroName))
- Check->warnMacro(MD);
+ Check->warnMacro(MD, MacroName);
if (CheckCapsOnly && !isCapsOnly(MacroName))
- Check->warnNaming(MD);
+ Check->warnNaming(MD, MacroName);
}
private:
MacroUsageCheck *Check;
+ const SourceManager &SM;
StringRef RegExp;
bool CheckCapsOnly;
+ bool IgnoreCommandLineMacros;
};
} // namespace
void MacroUsageCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowedRegexp", AllowedRegexp);
Options.store(Opts, "CheckCapsOnly", CheckCapsOnly);
+ Options.store(Opts, "IgnoreCommandLineMacros", IgnoreCommandLineMacros);
}
void MacroUsageCheck::registerPPCallbacks(CompilerInstance &Compiler) {
if (!getLangOpts().CPlusPlus11)
return;
Compiler.getPreprocessor().addPPCallbacks(
- llvm::make_unique(this, AllowedRegexp,
- CheckCapsOnly));
+ llvm::make_unique(this, Compiler.getSourceManager(),
+ AllowedRegexp, CheckCapsOnly,
+ IgnoreCommandLineMacros));
}
-void MacroUsageCheck::warnMacro(const MacroDirective *MD) {
+void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
StringRef Message =
- "macro used to declare a constant; consider using a 'constexpr' "
+ "macro '%0' used to declare a constant; consider using a 'constexpr' "
"constant";
/// A variadic macro is function-like at the same time. Therefore variadic
/// macros are checked first and will be excluded for the function-like
/// diagnostic.
if (MD->getMacroInfo()->isVariadic())
-Message = "variadic macro used; consider using a 'constexpr' "
+Message = "variadic macro '%0' used; consider using a 'constexpr' "
"variadic template function";
else if (MD->getMacroInfo()->isFunctionLike())
-Message = "function-like macro used; consider a 'constexpr' template "
+Message = "function-like macro '%0' used; consider a 'constexpr' template "
"function";
- diag(MD->getLocation(), Message);
+ diag(MD->getLocation(), Message) << MacroName;
}
-void MacroUsageCheck::warnNaming(const MacroDirective *MD) {
+void MacroUsageCheck::warnNaming(const MacroDirective *MD,
+ StringRef MacroName) {
diag(MD->getLocation(), "macro definition does not define the macro name "
- "using all uppercase characters");
+ "'%0' using all uppercase characters")
+ << MacroName;
}
} // namespace cppcoreguidelines
Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.h
==
[clang-tools-extra] r345610 - [clang-tidy] cppcoreguidelines-macro-usage: print macro names
Author: lebedevri
Date: Tue Oct 30 08:52:36 2018
New Revision: 345610
URL: http://llvm.org/viewvc/llvm-project?rev=345610&view=rev
Log:
[clang-tidy] cppcoreguidelines-macro-usage: print macro names
Summary:
The macro may not have location (or more generally, the location may not exist),
e.g. if it originates from compiler's command-line.
The check complains on all the macros, even those without the location info.
Which means, it only says it does not like it. What is 'it'? I have no idea.
If we don't print the name, then there is no way to deal with that situation.
And in general, not printing name here forces the user to try to understand,
given, the macro definition location, what is the macro name?
This isn't fun.
Also, ignores-by-default the macros originating from command-line,
with an option to not ignore those.
I suspect some more issues may crop up later.
Reviewers: JonasToth, aaron.ballman, hokein, xazax.hun, alexfh
Reviewed By: JonasToth, aaron.ballman
Subscribers: nemanjai, kbarton, rnkovacs, cfe-commits
Tags: #clang-tools-extra
Differential Revision: https://reviews.llvm.org/D53817
Added:
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-command-line-macros.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.h
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-macro-usage.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp?rev=345610&r1=345609&r2=345610&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
(original)
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
Tue Oct 30 08:52:36 2018
@@ -31,32 +31,41 @@ bool isCapsOnly(StringRef Name) {
class MacroUsageCallbacks : public PPCallbacks {
public:
- MacroUsageCallbacks(MacroUsageCheck *Check, StringRef RegExp, bool CapsOnly)
- : Check(Check), RegExp(RegExp), CheckCapsOnly(CapsOnly) {}
+ MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager &SM,
+ StringRef RegExp, bool CapsOnly, bool IgnoreCommandLine)
+ : Check(Check), SM(SM), RegExp(RegExp), CheckCapsOnly(CapsOnly),
+IgnoreCommandLineMacros(IgnoreCommandLine) {}
void MacroDefined(const Token &MacroNameTok,
const MacroDirective *MD) override {
if (MD->getMacroInfo()->isUsedForHeaderGuard() ||
MD->getMacroInfo()->getNumTokens() == 0)
return;
+if (IgnoreCommandLineMacros &&
+SM.isWrittenInCommandLineFile(MD->getLocation()))
+ return;
+
StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName();
if (!CheckCapsOnly && !llvm::Regex(RegExp).match(MacroName))
- Check->warnMacro(MD);
+ Check->warnMacro(MD, MacroName);
if (CheckCapsOnly && !isCapsOnly(MacroName))
- Check->warnNaming(MD);
+ Check->warnNaming(MD, MacroName);
}
private:
MacroUsageCheck *Check;
+ const SourceManager &SM;
StringRef RegExp;
bool CheckCapsOnly;
+ bool IgnoreCommandLineMacros;
};
} // namespace
void MacroUsageCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "AllowedRegexp", AllowedRegexp);
Options.store(Opts, "CheckCapsOnly", CheckCapsOnly);
+ Options.store(Opts, "IgnoreCommandLineMacros", IgnoreCommandLineMacros);
}
void MacroUsageCheck::registerPPCallbacks(CompilerInstance &Compiler) {
@@ -64,31 +73,34 @@ void MacroUsageCheck::registerPPCallback
return;
Compiler.getPreprocessor().addPPCallbacks(
- llvm::make_unique(this, AllowedRegexp,
- CheckCapsOnly));
+ llvm::make_unique(this, Compiler.getSourceManager(),
+ AllowedRegexp, CheckCapsOnly,
+ IgnoreCommandLineMacros));
}
-void MacroUsageCheck::warnMacro(const MacroDirective *MD) {
+void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName)
{
StringRef Message =
- "macro used to declare a constant; consider using a 'constexpr' "
+ "macro '%0' used to declare a constant; consider using a 'constexpr' "
"constant";
/// A variadic macro is function-like at the same time. Therefore variadic
/// macros are checked first and will be excluded for the function-like
/// diagnostic.
if (MD->getMacroInfo()->isVariadic())
-Message = "variadic macro used; con
[PATCH] D53814: Allow the analyzer to output to a SARIF file
george.karpenkov accepted this revision. george.karpenkov added a comment. This revision is now accepted and ready to land. I much prefer this version. We've had the same problem with diffing plist output. One thing we have learned is using a FileCheck was definitely a bad idea, as it leads to unreadable, unmaintainable, and very hard to update tests, so either diff or your custom tool is way better. As for the ultimate solution, I'm still not sure: I agree that maintaining those `-I` flags is annoying. One option is having an extra flag to produce "stable" output, which does not include absolute URLs/versions/etc. https://reviews.llvm.org/D53814 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet updated this revision to Diff 171717.
gchatelet marked 4 inline comments as done.
gchatelet added a comment.
- Remove leftover
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53488
Files:
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,5 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -- --
+// -target x86_64-unknown-linux
float ceil(float);
namespace std {
@@ -9,12 +10,12 @@
namespace floats {
struct ConvertsToFloat {
- operator float() const { return 0.5; }
+ operator float() const { return 0.5f; }
};
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
-void not_ok(double d) {
+void narrow_double_to_int_not_ok(double d) {
int i = 0;
i = d;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -24,11 +25,11 @@
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = ConvertsToFloat();
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i = 15_Pa;
+ i = 15_float;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
}
-void not_ok_binary_ops(double d) {
+void narrow_double_to_int_not_ok_binary_ops(double d) {
int i = 0;
i += 0.5;
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
@@ -52,6 +53,129 @@
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
}
+double operator"" _double(unsigned long long);
+
+float narrow_double_to_float_return() {
+ return 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok(double d) {
+ float f;
+ f = d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f = 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f = 15_double;
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_double_to_float_not_ok_binary_ops(double d) {
+ float f;
+ f += 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ // We warn on the following even though it's not dangerous because there is no reason to use a double literal here.
+ // TODO(courbet): Provide an automatic fix.
+ f += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+
+ f *= 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f /= 0.5;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+ f += (double)0.5f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'float' [cppcoreguidelines-narrowing-conversions]
+}
+
+void narrow_integer_to_floating() {
+ {
+long long ll; // 64 bits
+float f = ll; // doesn't fit in 24 bits
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'long long' to 'float' [cppcoreguidelines-narrowing-conversions]
+double d = ll; // doesn't fit in 53 bits.
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: narrowing conversion from 'long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+ }
+ {
+int i; // 32 bits
+float f = i; // doesn't fit in 24 bits
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: narrowing conversion from 'int' to 'float' [cppcoreguidelines-narrowing-conversions]
+double d = i; // fits in 53 bits.
+ }
+ {
+short s; // 16
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:25
// FIXME: Check double -> float truncation. Pay attention to casts:
void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
I think this comment is now outdated.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:42
+ unless(hasParent(castExpr())),
+ unless(isInTemplateInstantiation()))
+ .bind("cast"),
Here and in the matcher below:
`isInTemplateInstantiation` is a wide range, if you match only on
`isTypeDependent()` it would remove the false negatives from templates but
still catch clear cases that are within a template function.
With the current implementation non-type-templates would be ignored as well.
IMHO that could be done in a follow-up to keep the scope on one thing.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:66
+
+struct IntegerRange {
+ bool Contains(const IntegerRange &From) const {
Please enclose that `struct` with an anonymous namespace
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:85
+ if (T->isFloatingPoint()) {
+const unsigned PrecisionBits = llvm::APFloatBase::semanticsPrecision(
+Context.getFloatTypeSemantics(T->desugar()));
please remove that `const` as its uncommon to qualify values in LLVM code.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:92
+
+// 'One' is created with PrecisionBits plus two bytes:
+// - One to express the missing negative value of 2's complement
Maybe repleace `One` with `1.0`? It sounds slightly weird with the following
`One`s
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:118
+ if (ToType == FromType || ToType == nullptr || FromType == nullptr ||
+ ToType->isDependentType() || FromType->isDependentType())
+return false;
what about value dependentness? That is resolveable, but i believe ignored here
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:125
+ if (FromType->isIntegerType() && ToType->isIntegerType())
+return ToType->getScalarTypeKind() == clang::Type::STK_Bool
+ ? false
narrowing to `bool` is not nice either. I would prefer diagnosing these too.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:132
+
+ const auto FromIntegerRange = createFromType(Context, FromType);
+ const auto ToIntegerRange = createFromType(Context, ToType);
please no `const` and `auto` as well, as the type is not obvious.
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:147
+ const Expr *Rhs) {
+ const QualType LhsType = Lhs->getType();
+ const QualType RhsType = Rhs->getType();
please no `const`
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:149
+ const QualType RhsType = Rhs->getType();
+ if (Lhs->isValueDependent() || Rhs->isValueDependent() ||
+ LhsType->isDependentType() || RhsType->isDependentType())
you can shorten this condition with `isDependentType`
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:155
+return false;
+ const auto LhsIntegerRange = createFromType(Context, getBuiltinType(Lhs));
+ if (!LhsIntegerRange.Contains(IntegerConstant))
please no `const`
Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:165
+ if (const auto *CO = llvm::dyn_cast(Rhs)) {
+const bool NarrowLhs = diagIfNarrowConstant(
+Context, CO->getLHS()->getExprLoc(), Lhs, CO->getLHS());
you could remove both temporaries and return directly and then ellide the
braces.
If you don't like that please remove the `const`
Comment at:
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:15
-We enforce only part of the guideline, more specifically, we flag:
- - All floating-point to integer conversions that are not marked by an explicit
- cast (c-style or ``static_cast``). For example: ``int i = 0; i += 0.1;``,
- ``void f(int); f(0.1);``,
- - All applications of binary operators where the left-hand-side is an integer
- and the right-hand-size is a floating-point. For example:
- ``int i; i+= 0.1;``.
+A narrowing conversion is currently defined as a conversion from:
+ - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``),
Please reword that slightly. The narrowing conversions are defined by the
standard, we just implement it "partially" or so.
==
[PATCH] D53488: [clang-tidy] Catching narrowing from double to float.
gchatelet added a comment. Here are 15 random ones from **llvm** synced at `8f9fb8bab2e9b5b27fe40d700d2abe967b99fbb5`. lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3802:31: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] tools/dsymutil/MachOUtils.cpp:330:10: warning: narrowing conversion from 'unsigned long' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/X86/X86ISelDAGToDAG.cpp:1237:14: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] tools/llvm-objdump/MachODump.cpp:7998:12: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/CodeGen/MachinePipeliner.cpp:3268:11: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/CodeGen/MIRParser/MIRParser.cpp:823:41: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Analysis/MemoryBuiltins.cpp:610:59: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Analysis/ValueTracking.cpp:2291:21: warning: narrowing conversion from 'unsigned long' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/ARM/ARMLoadStoreOptimizer.cpp:1470:43: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2300:14: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/X86/X86TargetTransformInfo.cpp:2590:27: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/ARM/ARMFrameLowering.cpp:1770:27: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/ARM/ARMConstantIslandPass.cpp:514:22: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Transforms/Vectorize/LoopVectorize.cpp:5500:13: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1199:54: warning: narrowing conversion from 'int' to 'unsigned int' [cppcoreguidelines-narrowing-conversions] Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:14 #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/APInt.h" +#include "llvm/ADT/APSInt.h" Szelethus wrote: > Is `APInt` used anywhere? Good catch thx. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32 - const auto IsFloatExpr = - expr(hasType(realFloatingPointType()), unless(IsCeilFloorCall)); + const auto IsBuiltinOrEnumType = anyOf(builtinType(), enumType()); JonasToth wrote: > This matcher seems no to be used, did I overlook sth? It's a leftover indeed thx for noticing. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53522: [Frontend] Include module map header declaration in dependency file output
erik.pilkington updated this revision to Diff 171719.
erik.pilkington marked 7 inline comments as done.
erik.pilkington added a comment.
Address review comments. Thanks!
https://reviews.llvm.org/D53522
Files:
clang/include/clang/Lex/ModuleMap.h
clang/lib/Frontend/DependencyFile.cpp
clang/lib/Frontend/ModuleDependencyCollector.cpp
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/Inputs/dfg-symlinks.modulemap
clang/test/Modules/dependency-file-symlinks.c
clang/test/Modules/dependency-gen-pch.m
clang/test/Modules/dependency-gen.m
clang/test/Modules/relative-dep-gen.cpp
Index: clang/test/Modules/relative-dep-gen.cpp
===
--- clang/test/Modules/relative-dep-gen.cpp
+++ clang/test/Modules/relative-dep-gen.cpp
@@ -29,10 +29,8 @@
#include "Inputs/relative-dep-gen-1.h"
-// CHECK-BUILD: mod.pcm:
+// CHECK-BUILD: mod.pcm: {{.*}}Inputs{{[/\\]}}relative-dep-gen-1.h {{.*}}Inputs{{[/\\]}}relative-dep-gen-2.h
// CHECK-BUILD: {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen{{(-cwd)?}}.modulemap
-// CHECK-BUILD: {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen-1.h
-// CHECK-BUILD: {{[ \t]}}Inputs{{[/\\]}}relative-dep-gen-2.h
// CHECK-USE: use.o:
// CHECK-USE-DAG: {{[ \t]}}relative-dep-gen.cpp
// CHECK-EXPLICIT-DAG: mod.pcm
Index: clang/test/Modules/dependency-gen.m
===
--- clang/test/Modules/dependency-gen.m
+++ clang/test/Modules/dependency-gen.m
@@ -4,19 +4,19 @@
// RUN: %clang_cc1 -x objective-c -isystem %S/Inputs/System/usr/include -dependency-file %t.d.1 -MT %s.o -I %S/Inputs -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t-mcp %s
// RUN: FileCheck %s < %t.d.1
// CHECK: dependency-gen.m
-// CHECK: Inputs{{.}}module.map
// CHECK: Inputs{{.}}diamond_top.h
+// CHECK: Inputs{{.}}module.map
// CHECK-NOT: usr{{.}}include{{.}}module.map
// CHECK-NOT: stdint.h
// RUN: %clang_cc1 -x objective-c -isystem %S/Inputs/System/usr/include -dependency-file %t.d.2 -MT %s.o -I %S/Inputs -sys-header-deps -fsyntax-only -fmodules -fimplicit-module-maps -fmodules-cache-path=%t-mcp %s
// RUN: FileCheck %s -check-prefix=CHECK-SYS < %t.d.2
// CHECK-SYS: dependency-gen.m
-// CHECK-SYS: Inputs{{.}}module.map
// CHECK-SYS: Inputs{{.}}diamond_top.h
-// CHECK-SYS: usr{{.}}include{{.}}module.map
+// CHECK-SYS: Inputs{{.}}module.map
// CHECK-SYS: stdint.h
+// CHECK-SYS: usr{{.}}include{{.}}module.map
#import "diamond_top.h"
#import "stdint.h" // inside sysroot
Index: clang/test/Modules/dependency-gen-pch.m
===
--- clang/test/Modules/dependency-gen-pch.m
+++ clang/test/Modules/dependency-gen-pch.m
@@ -5,9 +5,9 @@
// RUN: %clang_cc1 -isysroot %S/Inputs/System -triple x86_64-apple-darwin10 -module-file-deps -dependency-file %t.d -MT %s.o -I %S/Inputs -fmodules -fimplicit-module-maps -fdisable-module-hash -fmodules-cache-path=%t-mcp -emit-pch -o %t.pch %s
// RUN: FileCheck %s < %t.d
// CHECK: dependency-gen-pch.m.o
-// CHECK-NEXT: dependency-gen-pch.m
-// CHECK-NEXT: Inputs{{.}}module.map
-// CHECK-NEXT: diamond_top.pcm
-// CHECK-NEXT: Inputs{{.}}diamond_top.h
+// CHECK: dependency-gen-pch.m
+// CHECK: Inputs{{.}}diamond_top.h
+// CHECK: Inputs{{.}}module.map
+// CHECK: diamond_top.pcm
#import "diamond_top.h"
Index: clang/test/Modules/dependency-file-symlinks.c
===
--- /dev/null
+++ clang/test/Modules/dependency-file-symlinks.c
@@ -0,0 +1,55 @@
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/cache
+
+// Set up %t as follows:
+// * misc.h #includes target.h
+// * target.h is empty
+// * link.h is a symlink to target.h
+
+// RUN: cp %S/Inputs/dfg-symlinks.modulemap %t/module.modulemap
+// RUN: echo "int foo();" > %t/target.h
+// RUN: ln -s %t/target.h %t/link.h
+// RUN: echo '#include "target.h"' >> %t/misc.h
+// RUN: echo 'int foo();' >> %t/misc.h
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %s \
+// RUN: -I %t -MT dependency-file-symlinks.o -dependency-file - | FileCheck --check-prefix=CHECK_IMP %s
+
+// Run clang again, to make sure that we get identical output with the cached
+// modules.
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache %s \
+// RUN: -I %t -MT dependency-file-symlinks.o -dependency-file - | FileCheck --check-prefix=CHECK_IMP %s
+
+// Verify that the an explicit use of a module via -fmodule-file doesn't include
+// any headers from that files's modulemap.
+
+// RUN: %clang_cc1 -fmodules -fmodule-name="my_module" -emit-module \
+// RUN: -o %t/my_module.pcm %t/module.modulemap
+
+// RUN: %clang_cc1 -fmodules -fmodule-file=%t/my_module.pcm %s -MT \
+// RUN: dependency-file-symlinks.o -I %t -dependency-file - | \
+// RUN: FileCheck --check-prefix CHECK_EXP %s
+
+#include "misc.h"
+
+int main() {
+ void *p = fo
[PATCH] D53522: [Frontend] Include module map header declaration in dependency file output
erik.pilkington added inline comments.
Comment at: clang/include/clang/Lex/ModuleMap.h:649-650
+ ///This can differ from \c Header's name due to symlinks.
void addHeader(Module *Mod, Module::Header Header,
- ModuleHeaderRole Role, bool Imported = false);
+ ModuleHeaderRole Role, StringRef OrigHeaderName = "",
+ bool Imported = false);
vsapsai wrote:
> How `OrigHeaderName` is different from `Module::Header.NameAsWritten`? Based
> on the names they should be the same, curious if and when it's not the case.
Oh, good point. I think NameAsWritten is what we're looking for here. Thanks!
Comment at: clang/lib/Frontend/DependencyFile.cpp:94
+ void moduleMapAddHeader(StringRef HeaderPath, bool IsSystem) override {
+if (!llvm::sys::path::is_absolute(HeaderPath))
+ return;
bruno wrote:
> Can you add a testecase for when `HeaderPath` isn't absolute?
Sure, new patch adds a test. This was meant to avoid including dependencies
found in -fmodule-files, but the new patch does this more directly by adding a
`bool Imported` parameter to this function. I sprinkled some comments around
explaining.
https://reviews.llvm.org/D53522
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined
martong marked 2 inline comments as done.
martong added a comment.
> I wonder if it is possible to get into situation where non-equivalent decls
> are marked equivalent with this patch? If yes, we can create a mapping
> between decls being imported and original decls as an alternative solution.
> However, I cannot find any counterexample.
I don't think so.
This change is the natural extension what we already do (in line 1021 and 1022)
with `getDefinition()`.
`getDefinition()` works just as we would expect with a simple `RecordDecl`:
`getDefinition()` returns a non-nullptr if `isBeingDefined()` is true.
However, `getDefinition()` may return a non-nullptr if `D` is a `CXXRecordDecl`
even if `D` is being defined.
CXXRecordDecl *getDefinition() const {
// We only need an update if we don't already know which
// declaration is the definition.
auto *DD = DefinitionData ? DefinitionData : dataPtr();
return DD ? DD->Definition : nullptr;
}
This all depends on whether `DefinitionData` is set. And we do set that during
`ImportDefinition(RecordDecl *,)`.
And then we start importing methods and fields of the `CXXRecordDecl` via
`ImportDeclContext` before the call to `completeDefinition()` which sets
`IsBeingDefined`.
During those imports, the `getDefinition()` of a `CXXRecordDecl` will return
with a non-nullptr value and we would go on checking the fields, but we are in
the middle of importing the fields (or methods).
Comment at: lib/AST/ASTStructuralEquivalence.cpp:1037
+ // equality and we assume that the decls are equal.
+ if (D1->isBeingDefined() || D2->isBeingDefined())
+return true;
a_sidorin wrote:
> Is it worth it to assert if only one Decl should be in `isBeingDefined()`
> state at time?
Strictly speaking `D1` will always come from the "From" context and such it
should report true for `isBeingDefined`. But the structural equivalence logic
should be independent from the import logic ideally, so I would not add that
assert.
Comment at: unittests/AST/ASTImporterTest.cpp:3729
+TEST_P(ASTImporterTestBase, ImportingTypedefShouldImportTheCompleteType) {
+ // We already have an incomplete underlying type in the "To" context.
a_sidorin wrote:
> Looks like this test is from another patch (D53693)?
Yes, exactly, good catch, I just removed it.
Repository:
rC Clang
https://reviews.llvm.org/D53697
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined
martong updated this revision to Diff 171723.
martong marked 2 inline comments as done.
martong added a comment.
- Remove unrelated test
Repository:
rC Clang
https://reviews.llvm.org/D53697
Files:
lib/AST/ASTStructuralEquivalence.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3726,6 +3726,45 @@
EXPECT_EQ(To1->getPreviousDecl(), To0);
}
+TEST_P(ASTImporterTestBase,
+ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+ B* b;
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+// We expect no (ODR) warning during the import.
+auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+ }
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
::testing::Values(ArgVector()), );
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -1016,7 +1016,8 @@
return false;
// Compare the definitions of these two records. If either or both are
- // incomplete, we assume that they are equivalent.
+ // incomplete (i.e. it is a forward decl), we assume that they are
+ // equivalent.
D1 = D1->getDefinition();
D2 = D2->getDefinition();
if (!D1 || !D2)
@@ -1031,6 +1032,11 @@
if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
return true;
+ // If one definition is currently being defined, we do not compare for
+ // equality and we assume that the decls are equal.
+ if (D1->isBeingDefined() || D2->isBeingDefined())
+return true;
+
if (auto *D1CXX = dyn_cast(D1)) {
if (auto *D2CXX = dyn_cast(D2)) {
if (D1CXX->hasExternalLexicalStorage() &&
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3726,6 +3726,45 @@
EXPECT_EQ(To1->getPreviousDecl(), To0);
}
+TEST_P(ASTImporterTestBase,
+ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+ B* b;
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+// We expect no (ODR) warning during the import.
+auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+ }
+}
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
::testing::Values(ArgVector()), );
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -1016,7 +1016,8 @@
return false;
// Compare the definitions of these two records. If either or both are
- // incomplete, we assume that they are equivalent.
+ // incomplete (i.e. it is a forward decl), we assume that they are
+ // equivalent.
D1 = D1->getDefinition();
D2 = D2->getDefinition();
if (!D1 || !D2)
@@ -1031,6 +1032,11 @@
if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
return true;
+ // If one definition is currently being defined, we do not com
[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible
ahatanak added a comment. http://wg21.link/p0968r0#2227 says: The destructor for each element of class type is potentially invoked (15.4 [class.dtor]) from the context where the aggregate initialization occurs. [Note: This provision ensures that destructors can be called for fully-constructed subobjects in case an exception is thrown (18.2 [except.ctor]). —end note] And '15.4 Destructors' has this sentence: A program is ill-formed if a destructor that is potentially invoked is deleted or not accessible from the context of the invocation. I'm not sure what "context" is in the case above (foo or Derived?), but if it's foo, it seems like clang is correct. Repository: rC Clang https://reviews.llvm.org/D53860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
lebedev.ri added inline comments.
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84
"cppcoreguidelines-c-copy-assignment-signature");
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-c-arrays");
JonasToth wrote:
> please conserve the alphabetical order here
Sorted all the `CheckFactories.registerCheck<>();` lines.
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:44
+ unless(anyOf(hasParent(varDecl(isExternC())),
+ hasAncestor(functionDecl(isExternC())
+ .bind("typeloc"),
JonasToth wrote:
> What about struct-decls that are externC?
Hm, what is a `struct-decl`?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
lebedev.ri updated this revision to Diff 171747.
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
Address review notes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/modernize/AvoidCArraysCheck.cpp
clang-tidy/modernize/AvoidCArraysCheck.h
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst
docs/clang-tidy/checks/hicpp-avoid-c-arrays.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
test/clang-tidy/modernize-avoid-c-arrays.cpp
Index: test/clang-tidy/modernize-avoid-c-arrays.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-avoid-c-arrays.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+
+int a[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int b[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+void foo() {
+ int c[b[0]];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+}
+
+template
+class array {
+ T d[Size];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+array d;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use std::array<> instead
+
+array dk;
+
+template
+class unique_ptr {
+ T *d;
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+unique_ptr d2;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+using k2 = int[];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+unique_ptr dk2;
+
+// Some header
+extern "C" {
+
+int f[] = {1, 2};
+
+int j[1];
+
+inline void bar() {
+ {
+int j[j[0]];
+ }
+}
+
+extern "C++" {
+int f3[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int j3[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+struct Foo {
+ int f3[3] = {1, 2};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+ int j3[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+}
+
+struct Bar {
+
+ int f[3] = {1, 2};
+
+ int j[1];
+};
+}
Index: docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - modernize-avoid-c-arrays
+
+modernize-avoid-c-arrays
+
+
+`cppcoreguidelines-avoid-c-arrays` redirects here as an alias for this check.
+
+`hicpp-avoid-c-arrays` redirects here as an alias for this check.
+
+Finds C-style array types and recommend to use ``std::array<>``.
+All types of C arrays are diagnosed.
+
+However, fix-it are potentially dangerous in header files and are therefore not
+emitted right now.
+
+.. code:: c++
+
+ int a[] = {1, 2}; // warning: do not declare C-style arrays, use std::array<> instead
+
+ int b[1]; // warning: do not declare C-style arrays, use std::array<> instead
+
+ void foo() {
+int c[b[0]]; // warning: do not declare C-style arrays, use std::array<> instead
+ }
+
+ template
+ class array {
+T d[Size]; // warning: do not declare C-style arrays, use std::array<> instead
+
+int e[1]; // warning: do not declare C-style arrays, use std::array<> instead
+ };
+
+ array d; // warning: do not declare C-style arrays, use std::array<> instead
+
+ using k = int[4]; // warning: do not declare C-style arrays, use std::array<> instead
+
+
+However, the ``extern "C"`` code is ignored, since it is common to share
+such headers between C code, and C++ code.
+
+.. code:: c++
+
+ // Some header
+ extern "C" {
+
+ int f[] = {1, 2}; // not diagnosed
+
+ int j[1]; // not diagnosed
+
+ inline void bar() {
+{
+ int j[j[0]]; // not diagnosed
+}
+ }
+
+ }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -87,6 +87,7 @@
cert-msc51
[PATCH] D53850: Declares __cpu_model as hidden symbol
hhb updated this revision to Diff 171748.
https://reviews.llvm.org/D53850
Files:
lib/CodeGen/CGBuiltin.cpp
test/CodeGen/builtin-cpu-is.c
test/CodeGen/builtin-cpu-supports.c
Index: test/CodeGen/builtin-cpu-supports.c
===
--- test/CodeGen/builtin-cpu-supports.c
+++ test/CodeGen/builtin-cpu-supports.c
@@ -4,6 +4,8 @@
// global, the bit grab, and the icmp correct.
extern void a(const char *);
+// CHECK: @__cpu_model = external hidden global { i32, i32, i32, [1 x i32] }
+
int main() {
__builtin_cpu_init();
Index: test/CodeGen/builtin-cpu-is.c
===
--- test/CodeGen/builtin-cpu-is.c
+++ test/CodeGen/builtin-cpu-is.c
@@ -4,6 +4,8 @@
// global, the bit grab, and the icmp correct.
extern void a(const char *);
+// CHECK: @__cpu_model = external hidden global { i32, i32, i32, [1 x i32] }
+
void intel() {
if (__builtin_cpu_is("intel"))
a("intel");
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -9016,6 +9016,8 @@
// Grab the global __cpu_model.
llvm::Constant *CpuModel = CGM.CreateRuntimeVariable(STy, "__cpu_model");
+ cast(CpuModel)->setVisibility(
+ llvm::GlobalValue::HiddenVisibility);
// Calculate the index needed to access the correct field based on the
// range. Also adjust the expected value.
@@ -9082,6 +9084,8 @@
// Grab the global __cpu_model.
llvm::Constant *CpuModel = CGM.CreateRuntimeVariable(STy, "__cpu_model");
+ cast(CpuModel)->setVisibility(
+ llvm::GlobalValue::HiddenVisibility);
// Grab the first (0th) element from the field __cpu_features off of the
// global in the struct STy.
Index: test/CodeGen/builtin-cpu-supports.c
===
--- test/CodeGen/builtin-cpu-supports.c
+++ test/CodeGen/builtin-cpu-supports.c
@@ -4,6 +4,8 @@
// global, the bit grab, and the icmp correct.
extern void a(const char *);
+// CHECK: @__cpu_model = external hidden global { i32, i32, i32, [1 x i32] }
+
int main() {
__builtin_cpu_init();
Index: test/CodeGen/builtin-cpu-is.c
===
--- test/CodeGen/builtin-cpu-is.c
+++ test/CodeGen/builtin-cpu-is.c
@@ -4,6 +4,8 @@
// global, the bit grab, and the icmp correct.
extern void a(const char *);
+// CHECK: @__cpu_model = external hidden global { i32, i32, i32, [1 x i32] }
+
void intel() {
if (__builtin_cpu_is("intel"))
a("intel");
Index: lib/CodeGen/CGBuiltin.cpp
===
--- lib/CodeGen/CGBuiltin.cpp
+++ lib/CodeGen/CGBuiltin.cpp
@@ -9016,6 +9016,8 @@
// Grab the global __cpu_model.
llvm::Constant *CpuModel = CGM.CreateRuntimeVariable(STy, "__cpu_model");
+ cast(CpuModel)->setVisibility(
+ llvm::GlobalValue::HiddenVisibility);
// Calculate the index needed to access the correct field based on the
// range. Also adjust the expected value.
@@ -9082,6 +9084,8 @@
// Grab the global __cpu_model.
llvm::Constant *CpuModel = CGM.CreateRuntimeVariable(STy, "__cpu_model");
+ cast(CpuModel)->setVisibility(
+ llvm::GlobalValue::HiddenVisibility);
// Grab the first (0th) element from the field __cpu_features off of the
// global in the struct STy.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC.
vsapsai updated this revision to Diff 171751.
vsapsai added a comment.
- Rename `EmitConstant` to `EmitScalarConstant`.
https://reviews.llvm.org/D53725
Files:
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/lib/CodeGen/CodeGenFunction.h
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -3524,6 +3524,7 @@
ConstantEmission tryEmitAsConstant(DeclRefExpr *refExpr);
ConstantEmission tryEmitAsConstant(const MemberExpr *ME);
+ llvm::Value *EmitScalarConstant(const ConstantEmission &Constant, Expr *E);
RValue EmitPseudoObjectRValue(const PseudoObjectExpr *e,
AggValueSlot slot = AggValueSlot::ignored());
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -456,19 +456,10 @@
return CGF.getOrCreateOpaqueRValueMapping(E).getScalarVal();
}
- Value *emitConstant(const CodeGenFunction::ConstantEmission &Constant,
- Expr *E) {
-assert(Constant && "not a constant");
-if (Constant.isReference())
- return EmitLoadOfLValue(Constant.getReferenceLValue(CGF, E),
- E->getExprLoc());
-return Constant.getValue();
- }
-
// l-values.
Value *VisitDeclRefExpr(DeclRefExpr *E) {
if (CodeGenFunction::ConstantEmission Constant = CGF.tryEmitAsConstant(E))
- return emitConstant(Constant, E);
+ return CGF.EmitScalarConstant(Constant, E);
return EmitLoadOfLValue(E);
}
@@ -1545,7 +1536,7 @@
Value *ScalarExprEmitter::VisitMemberExpr(MemberExpr *E) {
if (CodeGenFunction::ConstantEmission Constant = CGF.tryEmitAsConstant(E)) {
CGF.EmitIgnoredExpr(E->getBase());
-return emitConstant(Constant, E);
+return CGF.EmitScalarConstant(Constant, E);
} else {
llvm::APSInt Value;
if (E->EvaluateAsInt(Value, CGF.getContext(), Expr::SE_AllowSideEffects)) {
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1491,6 +1491,16 @@
return ConstantEmission();
}
+llvm::Value *CodeGenFunction::EmitScalarConstant(
+const CodeGenFunction::ConstantEmission &Constant, Expr *E) {
+ assert(Constant && "not a constant");
+ if (Constant.isReference())
+return EmitLoadOfLValue(Constant.getReferenceLValue(*this, E),
+E->getExprLoc())
+.getScalarVal();
+ return Constant.getValue();
+}
+
llvm::Value *CodeGenFunction::EmitLoadOfScalar(LValue lvalue,
SourceLocation Loc) {
return EmitLoadOfScalar(lvalue.getAddress(), lvalue.isVolatile(),
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -3524,6 +3524,7 @@
ConstantEmission tryEmitAsConstant(DeclRefExpr *refExpr);
ConstantEmission tryEmitAsConstant(const MemberExpr *ME);
+ llvm::Value *EmitScalarConstant(const ConstantEmission &Constant, Expr *E);
RValue EmitPseudoObjectRValue(const PseudoObjectExpr *e,
AggValueSlot slot = AggValueSlot::ignored());
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -456,19 +456,10 @@
return CGF.getOrCreateOpaqueRValueMapping(E).getScalarVal();
}
- Value *emitConstant(const CodeGenFunction::ConstantEmission &Constant,
- Expr *E) {
-assert(Constant && "not a constant");
-if (Constant.isReference())
- return EmitLoadOfLValue(Constant.getReferenceLValue(CGF, E),
- E->getExprLoc());
-return Constant.getValue();
- }
-
// l-values.
Value *VisitDeclRefExpr(DeclRefExpr *E) {
if (CodeGenFunction::ConstantEmission Constant = CGF.tryEmitAsConstant(E))
- return emitConstant(Constant, E);
+ return CGF.EmitScalarConstant(Constant, E);
return EmitLoadOfLValue(E);
}
@@ -1545,7 +1536,7 @@
Value *ScalarExprEmitter::VisitMemberExpr(MemberExpr *E) {
if (CodeGenFunction::ConstantEmission Constant = CGF.tryEmitAsConstant(E)) {
CGF.EmitIgnoredExpr(E->getBase());
-return emitConstant(Constant, E);
+return CGF.EmitScalarConstant(Constant, E);
} else {
llvm::APSInt Value;
if (E->EvaluateAsInt(Value, CGF.getContext(), Expr::SE_AllowSideEffects)) {
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1491,6 +1491,16 @@
return Con
