r346583 - [clang]: Fix misapplied patch in 346582.
Author: kristina
Date: Sat Nov 10 00:04:38 2018
New Revision: 346583
URL: http://llvm.org/viewvc/llvm-project?rev=346583&view=rev
Log:
[clang]: Fix misapplied patch in 346582.
Modified:
cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=346583&r1=346582&r2=346583&view=diff
==
--- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp Sat Nov 10 00:04:38 2018
@@ -63,7 +63,7 @@ static void EmitDeclInit(CodeGenFunction
/// Emit code to cause the destruction of the given variable with
/// static storage duration.
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
-ConstantAddress addr) {
+ConstantAddress Addr) {
CodeGenModule &CGM = CGF.CGM;
// FIXME: __attribute__((cleanup)) ?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny requested changes to this revision. mgorny added a comment. This revision now requires changes to proceed. Also please remember to submit patches with `-U`, so that Phab has full context. Comment at: bindings/python/tests/cindex/test_cdb.py:42 +if HAS_FSPATH: +def test_lookup_succeed_pathlike(self): Please use conditional skipping instead. Maybe write a `@skip_if_no_fspath` decorator in `util.py` and use it for all tests. Skipping will have the advantage that instead of silently pretending we have less tests, we'd explicitly tell user why some of the tests aren't run. Comment at: bindings/python/tests/cindex/util.py:6 + +try: +import pathlib Wouldn't it be better to use `if HAS_FSPATH` here? In normal circumstances, the `pathlib` import will be unnecessarily attempted (and it will succeed if `pathlib` package is installed), even though it will never be used inside tests. Repository: rC Clang https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak updated this revision to Diff 173496.
jstasiak added a comment.
Tests are skipped using unittest skipping mechanism now and pathlib imported
only when necessary.
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindings/python/tests/cindex/test_code_completion.py
bindings/python/tests/cindex/test_translation_unit.py
bindings/python/tests/cindex/util.py
Index: bindings/python/tests/cindex/util.py
===
--- bindings/python/tests/cindex/util.py
+++ bindings/python/tests/cindex/util.py
@@ -1,5 +1,15 @@
# This file provides common utility functions for the test suite.
+import os
+HAS_FSPATH = hasattr(os, 'fspath')
+
+if HAS_FSPATH:
+from pathlib import Path as str_to_path
+else:
+str_to_path = None
+
+import unittest
+
from clang.cindex import Cursor
from clang.cindex import TranslationUnit
@@ -68,8 +78,13 @@
return cursors
+skip_if_no_fspath = unittest.skipUnless(HAS_FSPATH,
+"Requires file system path protocol / Python 3.6+")
+
__all__ = [
'get_cursor',
'get_cursors',
'get_tu',
+'skip_if_no_fspath',
+'str_to_path',
]
Index: bindings/python/tests/cindex/test_translation_unit.py
===
--- bindings/python/tests/cindex/test_translation_unit.py
+++ bindings/python/tests/cindex/test_translation_unit.py
@@ -20,6 +20,8 @@
from clang.cindex import TranslationUnit
from .util import get_cursor
from .util import get_tu
+from .util import skip_if_no_fspath
+from .util import str_to_path
kInputsDir = os.path.join(os.path.dirname(__file__), 'INPUTS')
@@ -36,6 +38,17 @@
yield t.name
+@contextmanager
+def save_tu_pathlike(tu):
+"""Convenience API to save a TranslationUnit to a file.
+
+Returns the filename it was saved to.
+"""
+with tempfile.NamedTemporaryFile() as t:
+tu.save(str_to_path(t.name))
+yield t.name
+
+
class TestTranslationUnit(unittest.TestCase):
def test_spelling(self):
path = os.path.join(kInputsDir, 'hello.cpp')
@@ -89,6 +102,22 @@
spellings = [c.spelling for c in tu.cursor.get_children()]
self.assertEqual(spellings[-1], 'x')
+@skip_if_no_fspath
+def test_from_source_accepts_pathlike(self):
+tu = TranslationUnit.from_source(str_to_path('fake.c'), ['-Iincludes'], unsaved_files = [
+(str_to_path('fake.c'), """
+#include "fake.h"
+int x;
+int SOME_DEFINE;
+"""),
+(str_to_path('includes/fake.h'), """
+#define SOME_DEFINE y
+""")
+])
+spellings = [c.spelling for c in tu.cursor.get_children()]
+self.assertEqual(spellings[-2], 'x')
+self.assertEqual(spellings[-1], 'y')
+
def assert_normpaths_equal(self, path1, path2):
""" Compares two paths for equality after normalizing them with
os.path.normpath
@@ -135,6 +164,16 @@
self.assertTrue(os.path.exists(path))
self.assertGreater(os.path.getsize(path), 0)
+@skip_if_no_fspath
+def test_save_pathlike(self):
+"""Ensure TranslationUnit.save() works with PathLike filename."""
+
+tu = get_tu('int foo();')
+
+with save_tu_pathlike(tu) as path:
+self.assertTrue(os.path.exists(path))
+self.assertGreater(os.path.getsize(path), 0)
+
def test_save_translation_errors(self):
"""Ensure that saving to an invalid directory raises."""
@@ -167,6 +206,22 @@
# Just in case there is an open file descriptor somewhere.
del tu2
+@skip_if_no_fspath
+def test_load_pathlike(self):
+"""Ensure TranslationUnits can be constructed from saved files -
+PathLike variant."""
+tu = get_tu('int foo();')
+self.assertEqual(len(tu.diagnostics), 0)
+with save_tu(tu) as path:
+tu2 = TranslationUnit.from_ast_file(filename=str_to_path(path))
+self.assertEqual(len(tu2.diagnostics), 0)
+
+foo = get_cursor(tu2, 'foo')
+self.assertIsNotNone(foo)
+
+# Just in case there is an open file descriptor somewhere.
+del tu2
+
def test_index_parse(self):
path = os.path.join(kInputsDir, 'hello.cpp')
index = Index.create()
@@ -185,6 +240,19 @@
with self.assertRaises(Exception):
f = tu.get_file('foobar.cpp')
+@skip_if_no_fspath
+def test_get_file_pathlike(self):
+"""Ensure tu.get_file() works appropriately with PathLike filenames."""
+
+tu = get_tu('int foo();')
+
+f = tu.get_file(str_to_path('t.c'))
+self.assertIsInstance(f, File)
+self.assertEqual(f.name, 't.c')
+
+with self.assertRaises(Exception):
+f = tu.get_file(str_to_path('foobar.cpp'))
+
def test_g
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak marked 2 inline comments as done. jstasiak added a comment. Thanks for the feedback, you're right those were things worth improving, I updated the code. https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32577: CMake: Replace open-coded find_package
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D32577 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
kristina added a comment.
I've managed to isolate enough to make for a testcase. Is that too broad or is
it okay to attach as is?
The following triggers the assert:
namespace util {
template
class test_vector
{
public:
test_vector(unsigned c)
: Used(0), TotalSize(sizeof(T) * c)
{
// Intentional
Storage = (T*)(new T[TotalSize]);
}
~test_vector() {
delete[] Storage;
}
char* data() {
return Storage;
}
unsigned size() {
return TotalSize;
}
void push_back(T one_thing) {
Storage[Used++] = one_thing;
}
private:
unsigned Used;
unsigned TotalSize;
T* Storage;
};
}
volatile int do_not_optimize_me = 0;
namespace os {
using char_vec_t = util::test_vector;
class logger_base
{
public:
constexpr logger_base() = default;
protected:
char_vec_t* NamePtr = nullptr;
char_vec_t Name= char_vec_t(10);
};
class logger : public logger_base
{
public:
void stuff(const char* fmt, int something) {
do_not_optimize_me = something + fmt[0];
}
logger()
{
Name = char_vec_t(8);
Name.push_back('a');
}
logger(const char* name)
{
Name = util::test_vector(__builtin_strlen(name));
Name.push_back(name[0]);
Name.push_back(name[1]);
}
};
}
#define TOPLEVEL_LOGGER(_var_name, _category_const) \
namespace { constexpr char $__##_var_name[] = _category_const; \
__attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
TOPLEVEL_LOGGER(s_log, "something");
class some_class {
public:
some_class(int some_value) {
do_not_optimize_me = some_value;
s_log.stuff("wawawa", 5 + do_not_optimize_me);
}
~some_class() = default;
};
static some_class s_ctor(1);
https://reviews.llvm.org/D54344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51531: [analyzer][UninitializedObjectChecker] Uninit regions are only reported once
Szelethus added a comment. Ping, @NoQ, do you insist on a global set? https://reviews.llvm.org/D51531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32578: CMake: Set LLVM_MAIN_INCLUDE_DIR to LLVM_INCLUDE_DIR
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D32578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32595: CMakeLists: Don't set LLVM_MAIN_SRC_DIR when building stand-alone clang
mgorny requested changes to this revision. mgorny added a comment. This revision now requires changes to proceed. Could you please rebase? I'm pretty sure this breaks our use but I can't test since it no longer applies cleanly. https://reviews.llvm.org/D32595 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: bindings/python/clang/cindex.py:133 +except AttributeError: +def fspath(string): +return string Optionally: this is now a little bit nitpick but could you use a different variable name (e.g. commonly used elsewhere `x`)? This could be a little bit confusing with Python's `string` module. https://reviews.llvm.org/D54120 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54120: [python] Support PathLike filenames and directories
jstasiak updated this revision to Diff 173505.
jstasiak added a comment.
That's fair, changed string to just x, should be obvious from the context what
x is.
Thank you for the review. As I don't have commit access I'd like to ask you to
commit this on my behalf.
https://reviews.llvm.org/D54120
Files:
bindings/python/clang/cindex.py
bindings/python/tests/cindex/test_cdb.py
bindings/python/tests/cindex/test_code_completion.py
bindings/python/tests/cindex/test_translation_unit.py
bindings/python/tests/cindex/util.py
Index: bindings/python/tests/cindex/util.py
===
--- bindings/python/tests/cindex/util.py
+++ bindings/python/tests/cindex/util.py
@@ -1,5 +1,15 @@
# This file provides common utility functions for the test suite.
+import os
+HAS_FSPATH = hasattr(os, 'fspath')
+
+if HAS_FSPATH:
+from pathlib import Path as str_to_path
+else:
+str_to_path = None
+
+import unittest
+
from clang.cindex import Cursor
from clang.cindex import TranslationUnit
@@ -68,8 +78,13 @@
return cursors
+skip_if_no_fspath = unittest.skipUnless(HAS_FSPATH,
+"Requires file system path protocol / Python 3.6+")
+
__all__ = [
'get_cursor',
'get_cursors',
'get_tu',
+'skip_if_no_fspath',
+'str_to_path',
]
Index: bindings/python/tests/cindex/test_translation_unit.py
===
--- bindings/python/tests/cindex/test_translation_unit.py
+++ bindings/python/tests/cindex/test_translation_unit.py
@@ -20,6 +20,8 @@
from clang.cindex import TranslationUnit
from .util import get_cursor
from .util import get_tu
+from .util import skip_if_no_fspath
+from .util import str_to_path
kInputsDir = os.path.join(os.path.dirname(__file__), 'INPUTS')
@@ -36,6 +38,17 @@
yield t.name
+@contextmanager
+def save_tu_pathlike(tu):
+"""Convenience API to save a TranslationUnit to a file.
+
+Returns the filename it was saved to.
+"""
+with tempfile.NamedTemporaryFile() as t:
+tu.save(str_to_path(t.name))
+yield t.name
+
+
class TestTranslationUnit(unittest.TestCase):
def test_spelling(self):
path = os.path.join(kInputsDir, 'hello.cpp')
@@ -89,6 +102,22 @@
spellings = [c.spelling for c in tu.cursor.get_children()]
self.assertEqual(spellings[-1], 'x')
+@skip_if_no_fspath
+def test_from_source_accepts_pathlike(self):
+tu = TranslationUnit.from_source(str_to_path('fake.c'), ['-Iincludes'], unsaved_files = [
+(str_to_path('fake.c'), """
+#include "fake.h"
+int x;
+int SOME_DEFINE;
+"""),
+(str_to_path('includes/fake.h'), """
+#define SOME_DEFINE y
+""")
+])
+spellings = [c.spelling for c in tu.cursor.get_children()]
+self.assertEqual(spellings[-2], 'x')
+self.assertEqual(spellings[-1], 'y')
+
def assert_normpaths_equal(self, path1, path2):
""" Compares two paths for equality after normalizing them with
os.path.normpath
@@ -135,6 +164,16 @@
self.assertTrue(os.path.exists(path))
self.assertGreater(os.path.getsize(path), 0)
+@skip_if_no_fspath
+def test_save_pathlike(self):
+"""Ensure TranslationUnit.save() works with PathLike filename."""
+
+tu = get_tu('int foo();')
+
+with save_tu_pathlike(tu) as path:
+self.assertTrue(os.path.exists(path))
+self.assertGreater(os.path.getsize(path), 0)
+
def test_save_translation_errors(self):
"""Ensure that saving to an invalid directory raises."""
@@ -167,6 +206,22 @@
# Just in case there is an open file descriptor somewhere.
del tu2
+@skip_if_no_fspath
+def test_load_pathlike(self):
+"""Ensure TranslationUnits can be constructed from saved files -
+PathLike variant."""
+tu = get_tu('int foo();')
+self.assertEqual(len(tu.diagnostics), 0)
+with save_tu(tu) as path:
+tu2 = TranslationUnit.from_ast_file(filename=str_to_path(path))
+self.assertEqual(len(tu2.diagnostics), 0)
+
+foo = get_cursor(tu2, 'foo')
+self.assertIsNotNone(foo)
+
+# Just in case there is an open file descriptor somewhere.
+del tu2
+
def test_index_parse(self):
path = os.path.join(kInputsDir, 'hello.cpp')
index = Index.create()
@@ -185,6 +240,19 @@
with self.assertRaises(Exception):
f = tu.get_file('foobar.cpp')
+@skip_if_no_fspath
+def test_get_file_pathlike(self):
+"""Ensure tu.get_file() works appropriately with PathLike filenames."""
+
+tu = get_tu('int foo();')
+
+f = tu.get_file(str_to_path('t.c'))
+self.assertIsInstance(f, File)
+self.assertEqual(f.name, 't.c')
+
+with self.asse
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
kristina updated this revision to Diff 173503.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
Revised, I think I worked out what was happening, there's an explanation in the
comment in the test. This is a relatively new attribute so the coverage for it
did not test this specific corner case, I've managed to manually narrow it down
and write a reliable regression test. The core issue boils down to having a
non-trivially destructible member in a superclass that lacks a destructor and a
subclass that lacks a destructor, in which case, a different path was taken to
balance out the static storage duration class' initializer call and the
`__cxa_atexit` registration. This adds an explicit check for the attribute and
avoids balancing out the constructor as intended by the attribute.
The new test successfully replicates the assertion failure and should fail for
the above cases in assertion builds. Without assertions the generated code
produces undefined behavior if the above conditions are met. There is a
separate test for this attribute that provides the coverage for its
functionality, this is a much more narrower test for a very specific scenario
in which it was possible to cause an improperly balanced constructor call
followed by a emission of a call to a destructor that would never be emitted
due to the attribute, thus tripping the assert. Now no attempt to call the
destructor is made if the declaration is marked as `no_destroy`.
`Test without patch:`
=> ninja check-clang-semacxx
FAIL: Clang :: SemaCXX/attr-no-destroy-d54344.cpp (164 of 818)
TEST 'Clang :: SemaCXX/attr-no-destroy-d54344.cpp'
FAILED
(--- stack trace and the expected assertion failure ---)
/q/org.llvm.caches/llvm-8.0/4141/tools/clang/test/SemaCXX/Output/attr-no-destroy-d54344.cpp.script:
line 1: 31356 Aborted (core dumped)
--
Testing Time: 5.03s
Failing Tests (1):
Clang :: SemaCXX/attr-no-destroy-d54344.cpp
Expected Passes: 816
Expected Failures : 1
Unexpected Failures: 1
FAILED: tools/clang/test/CMakeFiles/check-clang-semacxx
ninja: build stopped: subcommand failed.
`Patch applied:`
=> ninja check-clang-semacxx
Testing Time: 6.28s
Expected Passes: 817
Expected Failures : 1
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/SemaCXX/attr-no-destroy-d54344.cpp
Index: test/SemaCXX/attr-no-destroy-d54344.cpp
===
--- test/SemaCXX/attr-no-destroy-d54344.cpp
+++ test/SemaCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -S -O0 %s -o - > /dev/null
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+namespace util {
+ template
+ class test_vector
+ {
+ public:
+ test_vector(unsigned c)
+ : Used(0), TotalSize(sizeof(T) * c)
+ {
+ // Intentional
+ Storage = (T*)(new T[TotalSize]);
+ }
+ ~test_vector() {
+ delete[] Storage;
+ }
+ char* data() {
+ return Storage;
+ }
+ unsigned size() {
+ return TotalSize;
+ }
+ void push_back(T one_thing) {
+ Storage[Used++] = one_thing;
+ }
+ private:
+ unsigned Used;
+ unsigned TotalSize;
+ T* Storage;
+ };
+}
+
+volatile int do_not_optimize_me = 0;
+
+namespace os {
+ using char_vec_t = util::test_vector;
+
+ class logger_base
+ {
+ public:
+ constexpr logger_base() = default;
+ protected:
+ char_vec_t* NamePtr = nullptr;
+ char_vec_t Name= char_vec_t(10);
+ };
+
+ class logger : public logger_base
+ {
+ public:
+ void stuff(const char* fmt, int something) {
+ do_not_optimize_me = something + fmt[0];
+ }
+ logger()
+ {
+ Name = char_vec_t(8);
+ Name.push_back('a');
+ }
+
+ logger(const char* name)
+ {
+ Name = util::test_vector(__builtin_strlen(name));
+ Name.push_back(name[0]);
+ Name.push_back(name[1]);
+ }
+ };
+}
+
+#define TOPLEVEL_LOGGER(_var_name, _category_const) \
+ namespace { constexpr char $__##_var_name[] = _category_const; \
+ __attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
+
+TOPLEVEL_LOGGER(s_log, "something");
+
+class some_class {
+public:
+ some_class(int some_value) {
+ do_not_optimize_me = some_value;
+ s_log.stuff("wawawa", 5 + do_not_optimize_me);
+ }
+ ~some_class() = default;
+};
+
+static some_class s_ctor(1);
\ No newline at end of file
Index: lib/CodeGen/CGDeclCXX.cpp
==
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
lebedev.ri added inline comments. Comment at: test/SemaCXX/attr-no-destroy-d54344.cpp:93 +static some_class s_ctor(1); \ No newline at end of file Please add new line. Repository: rC Clang https://reviews.llvm.org/D54344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. > I'll run it on LibreOffice code again and we'll see. Perfect, if you have the time, running it against LLVM would be very interesting as well. >> Do you have commit access? > > Commit access? This is my first patch. If you plan to regularly contribute to LLVM you can ask for commit access (process written here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access). I (or someone else) can commit this patch for you in the meanwhile (of course with attribution to you). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
kristina updated this revision to Diff 173507.
kristina added a comment.
Revised, added a newline to the testcase.
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/SemaCXX/attr-no-destroy-d54344.cpp
Index: test/SemaCXX/attr-no-destroy-d54344.cpp
===
--- test/SemaCXX/attr-no-destroy-d54344.cpp
+++ test/SemaCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,92 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -S -O0 %s -o - > /dev/null
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+namespace util {
+ template
+ class test_vector
+ {
+ public:
+ test_vector(unsigned c)
+ : Used(0), TotalSize(sizeof(T) * c)
+ {
+ // Intentional
+ Storage = (T*)(new T[TotalSize]);
+ }
+ ~test_vector() {
+ delete[] Storage;
+ }
+ char* data() {
+ return Storage;
+ }
+ unsigned size() {
+ return TotalSize;
+ }
+ void push_back(T one_thing) {
+ Storage[Used++] = one_thing;
+ }
+ private:
+ unsigned Used;
+ unsigned TotalSize;
+ T* Storage;
+ };
+}
+
+volatile int do_not_optimize_me = 0;
+
+namespace os {
+ using char_vec_t = util::test_vector;
+
+ class logger_base
+ {
+ public:
+ constexpr logger_base() = default;
+ protected:
+ char_vec_t* NamePtr = nullptr;
+ char_vec_t Name= char_vec_t(10);
+ };
+
+ class logger : public logger_base
+ {
+ public:
+ void stuff(const char* fmt, int something) {
+ do_not_optimize_me = something + fmt[0];
+ }
+ logger()
+ {
+ Name = char_vec_t(8);
+ Name.push_back('a');
+ }
+
+ logger(const char* name)
+ {
+ Name = util::test_vector(__builtin_strlen(name));
+ Name.push_back(name[0]);
+ Name.push_back(name[1]);
+ }
+ };
+}
+
+#define TOPLEVEL_LOGGER(_var_name, _category_const) \
+ namespace { constexpr char $__##_var_name[] = _category_const; \
+ __attribute((no_destroy)) os::logger _var_name ($__##_var_name) ; }
+
+TOPLEVEL_LOGGER(s_log, "something");
+
+class some_class {
+public:
+ some_class(int some_value) {
+ do_not_optimize_me = some_value;
+ s_log.stuff("wawawa", 5 + do_not_optimize_me);
+ }
+ ~some_class() = default;
+};
+
+static some_class s_ctor(1);
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,14 @@
/// static storage duration.
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
ConstantAddress Addr) {
+ // Honor __attribute__((no_destroy)) and bail instead of attempting
+ // to emit a reference to a possibly non-existant destructor, which
+ // in turn can cause a crash. This will result in a global constructor
+ // that isn't balanced out by a destructor call as intended by the
+ // attribute (D54344).
+ if (D.hasAttr())
+return;
+
CodeGenModule &CGM = CGF.CGM;
// FIXME: __attribute__((cleanup)) ?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346586 - [python] Support PathLike filenames and directories
Author: mgorny
Date: Sat Nov 10 03:41:36 2018
New Revision: 346586
URL: http://llvm.org/viewvc/llvm-project?rev=346586&view=rev
Log:
[python] Support PathLike filenames and directories
Python 3.6 introduced a file system path protocol (PEP 519[1]).
The standard library APIs accepting file system paths now accept path
objects too. It could be useful to add this here as well
for convenience.
[1] https://www.python.org/dev/peps/pep-0519
Authored by: jstasiak (Jakub Stasiak)
Differential Revision: https://reviews.llvm.org/D54120
Modified:
cfe/trunk/bindings/python/clang/cindex.py
cfe/trunk/bindings/python/tests/cindex/test_cdb.py
cfe/trunk/bindings/python/tests/cindex/test_code_completion.py
cfe/trunk/bindings/python/tests/cindex/test_translation_unit.py
cfe/trunk/bindings/python/tests/cindex/util.py
Modified: cfe/trunk/bindings/python/clang/cindex.py
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/bindings/python/clang/cindex.py?rev=346586&r1=346585&r2=346586&view=diff
==
--- cfe/trunk/bindings/python/clang/cindex.py (original)
+++ cfe/trunk/bindings/python/clang/cindex.py Sat Nov 10 03:41:36 2018
@@ -67,6 +67,7 @@ import collections
import clang.enumerations
+import os
import sys
if sys.version_info[0] == 3:
# Python 3 strings are unicode, translate them to/from utf8 for C-interop.
@@ -123,6 +124,14 @@ elif sys.version_info[0] == 2:
def b(x):
return x
+# We only support PathLike objects on Python version with os.fspath present
+# to be consistent with the Python standard library. On older Python versions
+# we only support strings and we have dummy fspath to just pass them through.
+try:
+fspath = os.fspath
+except AttributeError:
+def fspath(x):
+return x
# ctypes doesn't implicitly convert c_void_p to the appropriate wrapper
# object. This is a problem, because it means that from_parameter will see an
@@ -2752,11 +2761,11 @@ class TranslationUnit(ClangObject):
etc. e.g. ["-Wall", "-I/path/to/include"].
In-memory file content can be provided via unsaved_files. This is an
-iterable of 2-tuples. The first element is the str filename. The
-second element defines the content. Content can be provided as str
-source code or as file objects (anything with a read() method). If
-a file object is being used, content will be read until EOF and the
-read cursor will not be reset to its original position.
+iterable of 2-tuples. The first element is the filename (str or
+PathLike). The second element defines the content. Content can be
+provided as str source code or as file objects (anything with a read()
+method). If a file object is being used, content will be read until EOF
+and the read cursor will not be reset to its original position.
options is a bitwise or of TranslationUnit.PARSE_XXX flags which will
control parsing behavior.
@@ -2801,11 +2810,13 @@ class TranslationUnit(ClangObject):
if hasattr(contents, "read"):
contents = contents.read()
-unsaved_array[i].name = b(name)
+unsaved_array[i].name = b(fspath(name))
unsaved_array[i].contents = b(contents)
unsaved_array[i].length = len(contents)
-ptr = conf.lib.clang_parseTranslationUnit(index, filename, args_array,
+ptr = conf.lib.clang_parseTranslationUnit(index,
+fspath(filename) if filename is not None
else None,
+args_array,
len(args), unsaved_array,
len(unsaved_files), options)
@@ -2826,11 +2837,13 @@ class TranslationUnit(ClangObject):
index is optional and is the Index instance to use. If not provided,
a default Index will be created.
+
+filename can be str or PathLike.
"""
if index is None:
index = Index.create()
-ptr = conf.lib.clang_createTranslationUnit(index, filename)
+ptr = conf.lib.clang_createTranslationUnit(index, fspath(filename))
if not ptr:
raise TranslationUnitLoadError(filename)
@@ -2983,7 +2996,7 @@ class TranslationUnit(ClangObject):
print(value)
if not isinstance(value, str):
raise TypeError('Unexpected unsaved file contents.')
-unsaved_files_array[i].name = name
+unsaved_files_array[i].name = fspath(name)
unsaved_files_array[i].contents = value
unsaved_files_array[i].length = len(value)
ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
@@ -3002,10 +3015,10 @@ class TranslationUnit(ClangObject):
case, the reason(s) why should be available via
[PATCH] D54120: [python] Support PathLike filenames and directories
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346586: [python] Support PathLike filenames and directories
(authored by mgorny, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54120?vs=173505&id=173508#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54120
Files:
cfe/trunk/bindings/python/clang/cindex.py
cfe/trunk/bindings/python/tests/cindex/test_cdb.py
cfe/trunk/bindings/python/tests/cindex/test_code_completion.py
cfe/trunk/bindings/python/tests/cindex/test_translation_unit.py
cfe/trunk/bindings/python/tests/cindex/util.py
Index: cfe/trunk/bindings/python/clang/cindex.py
===
--- cfe/trunk/bindings/python/clang/cindex.py
+++ cfe/trunk/bindings/python/clang/cindex.py
@@ -67,6 +67,7 @@
import clang.enumerations
+import os
import sys
if sys.version_info[0] == 3:
# Python 3 strings are unicode, translate them to/from utf8 for C-interop.
@@ -123,6 +124,14 @@
def b(x):
return x
+# We only support PathLike objects on Python version with os.fspath present
+# to be consistent with the Python standard library. On older Python versions
+# we only support strings and we have dummy fspath to just pass them through.
+try:
+fspath = os.fspath
+except AttributeError:
+def fspath(x):
+return x
# ctypes doesn't implicitly convert c_void_p to the appropriate wrapper
# object. This is a problem, because it means that from_parameter will see an
@@ -2752,11 +2761,11 @@
etc. e.g. ["-Wall", "-I/path/to/include"].
In-memory file content can be provided via unsaved_files. This is an
-iterable of 2-tuples. The first element is the str filename. The
-second element defines the content. Content can be provided as str
-source code or as file objects (anything with a read() method). If
-a file object is being used, content will be read until EOF and the
-read cursor will not be reset to its original position.
+iterable of 2-tuples. The first element is the filename (str or
+PathLike). The second element defines the content. Content can be
+provided as str source code or as file objects (anything with a read()
+method). If a file object is being used, content will be read until EOF
+and the read cursor will not be reset to its original position.
options is a bitwise or of TranslationUnit.PARSE_XXX flags which will
control parsing behavior.
@@ -2801,11 +2810,13 @@
if hasattr(contents, "read"):
contents = contents.read()
-unsaved_array[i].name = b(name)
+unsaved_array[i].name = b(fspath(name))
unsaved_array[i].contents = b(contents)
unsaved_array[i].length = len(contents)
-ptr = conf.lib.clang_parseTranslationUnit(index, filename, args_array,
+ptr = conf.lib.clang_parseTranslationUnit(index,
+fspath(filename) if filename is not None else None,
+args_array,
len(args), unsaved_array,
len(unsaved_files), options)
@@ -2826,11 +2837,13 @@
index is optional and is the Index instance to use. If not provided,
a default Index will be created.
+
+filename can be str or PathLike.
"""
if index is None:
index = Index.create()
-ptr = conf.lib.clang_createTranslationUnit(index, filename)
+ptr = conf.lib.clang_createTranslationUnit(index, fspath(filename))
if not ptr:
raise TranslationUnitLoadError(filename)
@@ -2983,7 +2996,7 @@
print(value)
if not isinstance(value, str):
raise TypeError('Unexpected unsaved file contents.')
-unsaved_files_array[i].name = name
+unsaved_files_array[i].name = fspath(name)
unsaved_files_array[i].contents = value
unsaved_files_array[i].length = len(value)
ptr = conf.lib.clang_reparseTranslationUnit(self, len(unsaved_files),
@@ -3002,10 +3015,10 @@
case, the reason(s) why should be available via
TranslationUnit.diagnostics().
-filename -- The path to save the translation unit to.
+filename -- The path to save the translation unit to (str or PathLike).
"""
options = conf.lib.clang_defaultSaveOptions(self)
-result = int(conf.lib.clang_saveTranslationUnit(self, filename,
+result = int(conf.lib.clang_saveTranslationUnit(self, fspath(filename),
options))
if result != 0:
raise TranslationUnitSaveError(result,
@@ -3047,10 +3060,10 @@
[PATCH] D54120: [python] Support PathLike filenames and directories
mgorny added a comment. Merged. I will get back to you if something explodes ;-). Repository: rL LLVM https://reviews.llvm.org/D54120 ___ 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 a comment. Comments? :) 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
JonasToth added inline comments. Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:26 + const clang::Type *TypeNode = Node.getTypePtr(); + return (TypeNode != nullptr && + InnerMatcher.matches(*TypeNode, Finder, Builder)); Nit: these parens are superflous Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:64 + diag(ArrayType->getBeginLoc(), + isa(ArrayType->getTypePtr()) ? UseVector : UseArray); +} Why `isa<>` and not `isVariableArrayType()` (does it exist?) `isa<>` would not resolve typedefs, but I think they should be considered. 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] D54379: Add Hurd support
sthibaul created this revision.
sthibaul added a reviewer: rengolin.
Herald added subscribers: cfe-commits, fedor.sergeev, krytarowski, mgorny.
Repository:
rC Clang
https://reviews.llvm.org/D54379
Files:
lib/Basic/Targets.cpp
lib/Basic/Targets/OSTargets.h
lib/Driver/CMakeLists.txt
lib/Driver/Driver.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,36 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+ Hurd(const Driver &D, const llvm::Triple &Triple,
+ const llvm::opt::ArgList &Args);
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+llvm::opt::ArgStringList &CC1Args) const override;
+
+ virtual std::string computeSysRoot() const;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,142 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+/// Get our best guess at the multiarch triple for a target.
+///
+/// Debian-based systems are starting to use a multiarch setup where they use
+/// a target-triple directory in the library and header search paths.
+/// Unfortunately, this triple does not align with the vanilla target triple,
+/// so we provide a rough mapping here.
+static std::string getMultiarchTriple(const Driver &D,
+ const llvm::Triple &TargetTriple,
+ StringRef SysRoot) {
+ // For most architectures, just use whatever we have rather than trying to be
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
+break;
+
+ // We use the existence of '/lib/' as a directory to detect some
+ // common hurd triples that don't quite match the Clang triple for both
+ // 32-bit and 64-bit targets. Multiarch fixes its install triples to these
+ // regardless of what the actual target triple is.
+ case llvm::Triple::x86:
+if (D.getVFS().exists(SysRoot + "/lib/i386-gnu"))
+ return "i386-gnu";
+break;
+ }
+
+ return TargetTriple.str();
+}
+
+static StringRef getOSLibDir(const llvm::Triple &Triple, const ArgList &Args) {
+ // It happens that only x86 and PPC use the 'lib32' variant of oslibdir, and
+ // using that variant while targeting other architectures causes problems
+ // because the libraries are laid out in shared system roots that can't cope
+ // with a 'lib32' library search path being considered. So we only enable
+ // them when we know we may need it.
+ //
+ // FIXME: This is a bit of a hack. We should really unify this code for
+ // reasoning about oslibdir spellings with the lib dir spellings in the
+ // GCCInstallationDetector, but that is a more significant refactoring.
+
+ if (Triple.getArch() == llvm::Triple::x86)
+return "lib32";
+
+ return Triple.isArch32Bit() ? "lib" : "lib64";
+}
+
+Hurd::Hurd(const Driv
[PATCH] D54379: Add Hurd support
sthibaul added a comment. The Hurd::Hurd constructor would actually need to do the same gcc inclusion path detection as on Linux, but let's leave this aside for now, this commit is enough for a build without libc++. Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54379: Add Hurd support
kristina added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:533
- if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI) {
+ if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI ||
Triple.isOSHurd()) {
switch (Triple.getArch()) {
Please keep line lengths to 80 columns at most, it's one of the few hard rules
outlined in the developer policy.
Repository:
rC Clang
https://reviews.llvm.org/D54379
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in Szelethus wrote: > Make sure the **checker** list **is** updated I think at some point we should decide if we prefer the term check or checker to refer to these things :) Clang Tidy clearly prefers check. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in xazax.hun wrote: > Szelethus wrote: > > Make sure the **checker** list **is** updated > I think at some point we should decide if we prefer the term check or checker > to refer to these things :) Clang Tidy clearly prefers check. That is the distinction I'm aware of too: checkers in the Static Analyzer, checks in clang-tidy. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun updated this revision to Diff 173513. xazax.hun marked 11 inline comments as done. xazax.hun added a comment. - Move the checklist up before additional info in the HTML file. - Fix minor nits. - Add missing bullet points (thanks @Szelethus for noticing) I did not add any coding convention related item yet. I wonder if it is a good idea to have our own coding guidelines even if it is derived from the LLVM one. https://reviews.llvm.org/D52984 Files: www/analyzer/checker_dev_manual.html Index: www/analyzer/checker_dev_manual.html === --- www/analyzer/checker_dev_manual.html +++ www/analyzer/checker_dev_manual.html @@ -675,6 +675,111 @@ (gdb) p C.getPredecessor()->getCodeDecl().getBody()->dump() +Making Your Check Better + +User facing documentation is important for adoption! Make sure the check list is updated +at the homepage of the analyzer. Also ensure the description is clear to +non-analyzer-developers in Checkers.td. +Warning and note messages should be clear and easy to understand, even if a bit long. + + Messages should start with a capital letter (unlike Clang warnings!) and should not + end with .. + Articles are usually omitted, eg. Dereference of a null pointer -> + Dereference of null pointer. + Introduce BugReporterVisitors to emit additional notes that explain the warning + to the user better. There are some existing visitors that might be useful for your check, + e.g. trackNullOrUndefValue. For example, SimpleStreamChecker should highlight + the event of opening the file when reporting a file descriptor leak. + +If the check tracks anything in the program state, it needs to implement the +checkDeadSymbolscallback to clean the state up. +The check should conservatively assume that the program is correct when a tracked symbol +is passed to a function that is unknown to the analyzer. +checkPointerEscape callback could help you handle that case. +Use safe and convenient APIs! + + Always use CheckerContext::generateErrorNode and +CheckerContext::generateNonFatalErrorNode for emitting bug reports. +Most importantly, never emit report against CheckerContext::getPredecessor. + Prefer checkPreCall and checkPostCall to +checkPreStmtand checkPostStmt . + Use CallDescription to detect hardcoded API calls in the program. + Simplify C.getState()->getSVal(E, C.getLocationContext()) to C.getSVal(E). + +Common sources of crashes: + + CallEvent::getOriginExpr is nullable - for example, it returns null for an +automatic destructor of a variable. The same applies to some values generated while the +call was modeled, eg. SymbolConjured::getStmt is nullable. + CallEvent::getDecl is nullable - for example, it returns null for a + call of symbolic function pointer. + addTransition, generateSink, generateNonFatalErrorNode, +generateErrorNode are nullable because you can transition to a node that you have already visited. + Methods of CallExpr/FunctionDecl/CallEvent that +return arguments crash when the argument is out-of-bounds. If you checked the function name, +it doesn't mean that the function has the expected number of arguments! +Which is why you should use CallDescription. + Nullability of different entities within different kinds of symbols and regions is usually + documented via assertions in their constructors. + NamedDecl::getName will fail if the name of the declaration is not a single token, +e.g. for destructors. You could use NamedDecl::getNameAsString for those cases. +Note that this method is much slower and should be used sparringly, e.g. only when generating reports +but not during analysis. + Is -analyzer-checker=core included in all test RUN: lines? It was never supported +to run the analyzer with the core checks disabled. It might cause unexpected behavior and +crashes. You should do all your testing with the core checks enabled. + + +Patterns that you should most likely avoid even if they're not technically wrong: + + BugReporterVisitor should most likely not match the AST of the current program point + to decide when to emit a note. It is much easier to determine that by observing changes in + the program state. + In State->getSVal(Region), Region is not necessarily a TypedValueRegion + and the optional type argument is not specified. It is likely that the checker + may accidentally try to dereference a void pointer. + Checker logic depends on whether a certain value is a Loc or NonLoc. +It should be immediately obvious whether the SVal is a Loc or a +NonLoc depending on the AST that is being checked. Checking whether a value +is Loc or Unknown/Undefined or whether the value is +NonLoc or Unknown/Undefined is totally fine. + New symbols are constructed in the checker via direct calls to SymbolManager, +unless they are o
[PATCH] D52984: [analyzer] Checker reviewer's checklist
xazax.hun added inline comments. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in Szelethus wrote: > xazax.hun wrote: > > Szelethus wrote: > > > Make sure the **checker** list **is** updated > > I think at some point we should decide if we prefer the term check or > > checker to refer to these things :) Clang Tidy clearly prefers check. > That is the distinction I'm aware of too: checkers in the Static Analyzer, > checks in clang-tidy. My understanding is the following: we want users to use the term `check`, since that is more widespread and used by other (non-clang) tools as well. The term `checker` is something like a historical artifact in the developer community of the static analyzer. But if this is not the case, I am happy to change the terminology. But I do want to have some input from rest of the community too :) https://reviews.llvm.org/D52984 ___ 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/modernize/AvoidCArraysCheck.cpp:64 + diag(ArrayType->getBeginLoc(), + isa(ArrayType->getTypePtr()) ? UseVector : UseArray); +} JonasToth wrote: > Why `isa<>` and not `isVariableArrayType()` (does it exist?) > `isa<>` would not resolve typedefs, but I think they should be considered. Hmm, interesting point. I'm not sure ATM how to expose it in tests, but it counts as a cleanup anyway. 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 173514.
lebedev.ri marked 2 inline comments as done.
lebedev.ri added a comment.
Use `isVariableArrayType()`
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,88 @@
+// 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 VLA arrays, use std::vector<> instead
+
+ using d = decltype(c);
+ d e;
+ // Semi-FIXME: we do not diagnose these last two lines separately,
+ // because we point at typeLoc.getBeginLoc(), which is the decl before that
+ // (int c[b[0]];), which is already diagnosed.
+}
+
+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<>`` /
+``std::vector<>``. 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 VLA arrays, use std::vector<> 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() {
+{
+ i
[PATCH] D53771: [clang-tidy] Avoid C arrays check
JonasToth added a comment. from my side mostly ok. I think the typedef points should be clarified, but I dont see more issues. Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:32 + +using k = int[4]; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use std::array<> instead What happens for a variable of type `k`? Does the check warn, or only in the typedef? 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] D52984: [analyzer] Checker reviewer's checklist
aaron.ballman added a comment. In https://reviews.llvm.org/D52984#1294223, @xazax.hun wrote: > - Move the checklist up before additional info in the HTML file. > - Fix minor nits. > - Add missing bullet points (thanks @Szelethus for noticing) > > I did not add any coding convention related item yet. I wonder if it is a > good idea to have our own coding guidelines even if it is derived from the > LLVM one. Personally, I think it's detrimental to the community for subprojects to come up with their own coding guidelines. My preference is for the static analyzer to come more in line with the rest of the project (over time, organically) in terms of style, terminology, diagnostic wording, etc. However, if the consensus is that we want a separate coding standard, I think it should be explicitly documented somewhere public and then maintained as part of the project. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in xazax.hun wrote: > Szelethus wrote: > > xazax.hun wrote: > > > Szelethus wrote: > > > > Make sure the **checker** list **is** updated > > > I think at some point we should decide if we prefer the term check or > > > checker to refer to these things :) Clang Tidy clearly prefers check. > > That is the distinction I'm aware of too: checkers in the Static Analyzer, > > checks in clang-tidy. > My understanding is the following: we want users to use the term `check`, > since that is more widespread and used by other (non-clang) tools as well. > The term `checker` is something like a historical artifact in the developer > community of the static analyzer. But if this is not the case, I am happy to > change the terminology. But I do want to have some input from rest of the > community too :) I grew up with the term "checker", but I feel like "check" may have won the war. I don't have a strong opinion here though. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54379: Add Hurd support
kristina added reviewers: kristina, clang.
kristina added a comment.
A few style naming/comments.
Comment at: lib/Driver/ToolChains/Hurd.cpp:36
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
Does this need a switch? Wouldn't an `if` be sufficient?
Comment at: lib/Driver/ToolChains/Hurd.cpp:106
+CIncludeDirs.split(dirs, ":");
+for (StringRef dir : dirs) {
+ StringRef Prefix =
Variable names should be capitalized.
Comment at: lib/Driver/ToolChains/Hurd.cpp:125
+break;
+ default:
+break;
Default should generally be the first case, if present.
Repository:
rC Clang
https://reviews.llvm.org/D54379
___
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: test/clang-tidy/modernize-avoid-c-arrays.cpp:32 + +using k = int[4]; +// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use std::array<> instead JonasToth wrote: > What happens for a variable of type `k`? Does the check warn, or only in the > typedef? Only here, on the typeloc, like the `Semi-FIXME` i just added there ^ states. 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
JonasToth accepted this revision. JonasToth added a comment. This revision is now accepted and ready to land. LGTM. Please give other reviewers a chance to take a look at it too. 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 added a comment. *Maybe* we should be actually diagnosing the each actual declaration, not just the `typeloc`. Else if you use one `typedef`, and `// NOLINT` it, you will silence it for all the decls that use it. In https://reviews.llvm.org/D53771#1294241, @JonasToth wrote: > LGTM. > Please give other reviewers a chance to take a look at it too. Thank you for the review! I will keep this open for a few more days. 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] D54379: Add Hurd support
sthibaul marked 3 inline comments as done.
sthibaul added a comment.
I commented one of them, and will fix the rest.
Comment at: lib/Driver/ToolChains/Hurd.cpp:36
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
kristina wrote:
> Does this need a switch? Wouldn't an `if` be sufficient?
An if would work, yes, it's just to make it easily extensible for future cases,
just like the getMultiarchTriple function in Linux.cpp
Repository:
rC Clang
https://reviews.llvm.org/D54379
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54379: Add Hurd support
sthibaul updated this revision to Diff 173515.
https://reviews.llvm.org/D54379
Files:
lib/Basic/Targets.cpp
lib/Basic/Targets/OSTargets.h
lib/Driver/CMakeLists.txt
lib/Driver/Driver.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/ToolChains/Hurd.cpp
lib/Driver/ToolChains/Hurd.h
lib/Frontend/InitHeaderSearch.cpp
Index: lib/Frontend/InitHeaderSearch.cpp
===
--- lib/Frontend/InitHeaderSearch.cpp
+++ lib/Frontend/InitHeaderSearch.cpp
@@ -260,6 +260,7 @@
switch (os) {
case llvm::Triple::Linux:
+ case llvm::Triple::Hurd:
case llvm::Triple::Solaris:
llvm_unreachable("Include management is handled in the driver.");
Index: lib/Driver/ToolChains/Hurd.h
===
--- lib/Driver/ToolChains/Hurd.h
+++ lib/Driver/ToolChains/Hurd.h
@@ -0,0 +1,36 @@
+//===--- Hurd.h - Hurd ToolChain Implementations --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
+
+#include "Gnu.h"
+#include "clang/Driver/ToolChain.h"
+
+namespace clang {
+namespace driver {
+namespace toolchains {
+
+class LLVM_LIBRARY_VISIBILITY Hurd : public Generic_ELF {
+public:
+ Hurd(const Driver &D, const llvm::Triple &Triple,
+ const llvm::opt::ArgList &Args);
+
+ void
+ AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+llvm::opt::ArgStringList &CC1Args) const override;
+
+ virtual std::string computeSysRoot() const;
+};
+
+} // end namespace toolchains
+} // end namespace driver
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_Hurd_H
Index: lib/Driver/ToolChains/Hurd.cpp
===
--- lib/Driver/ToolChains/Hurd.cpp
+++ lib/Driver/ToolChains/Hurd.cpp
@@ -0,0 +1,142 @@
+//===--- Hurd.cpp - Hurd ToolChain Implementations *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "Hurd.h"
+#include "CommonArgs.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Config/config.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Options.h"
+#include "llvm/Support/Path.h"
+
+using namespace clang::driver;
+using namespace clang::driver::toolchains;
+using namespace clang;
+using namespace llvm::opt;
+
+using tools::addPathIfExists;
+
+/// Get our best guess at the multiarch triple for a target.
+///
+/// Debian-based systems are starting to use a multiarch setup where they use
+/// a target-triple directory in the library and header search paths.
+/// Unfortunately, this triple does not align with the vanilla target triple,
+/// so we provide a rough mapping here.
+static std::string getMultiarchTriple(const Driver &D,
+ const llvm::Triple &TargetTriple,
+ StringRef SysRoot) {
+ // For most architectures, just use whatever we have rather than trying to be
+ // clever.
+ switch (TargetTriple.getArch()) {
+ default:
+break;
+
+ // We use the existence of '/lib/' as a directory to detect some
+ // common hurd triples that don't quite match the Clang triple for both
+ // 32-bit and 64-bit targets. Multiarch fixes its install triples to these
+ // regardless of what the actual target triple is.
+ case llvm::Triple::x86:
+if (D.getVFS().exists(SysRoot + "/lib/i386-gnu"))
+ return "i386-gnu";
+break;
+ }
+
+ return TargetTriple.str();
+}
+
+static StringRef getOSLibDir(const llvm::Triple &Triple, const ArgList &Args) {
+ // It happens that only x86 and PPC use the 'lib32' variant of oslibdir, and
+ // using that variant while targeting other architectures causes problems
+ // because the libraries are laid out in shared system roots that can't cope
+ // with a 'lib32' library search path being considered. So we only enable
+ // them when we know we may need it.
+ //
+ // FIXME: This is a bit of a hack. We should really unify this code for
+ // reasoning about oslibdir spellings with the lib dir spellings in the
+ // GCCInstallationDetector, but that is a more significant refactoring.
+
+ if (Triple.getArch() == llvm::Triple::x86)
+return "lib32";
+
+ return Triple.isArch32Bit() ? "lib" : "lib64";
+}
+
+Hurd::Hurd(const Driver &D, const llvm::Triple &Triple,
+ const ArgList &Args)
+: Generic_ELF(D, Triple, Args) { }
+
+std:
[PATCH] D53771: [clang-tidy] Avoid C arrays check
JonasToth added a comment. In https://reviews.llvm.org/D53771#1294244, @lebedev.ri wrote: > *Maybe* we should be actually diagnosing the each actual declaration, not > just the `typeloc`. > Else if you use one `typedef`, and `// NOLINT` it, you will silence it for > all the decls that use it. As my BB is getting closer to being useful, we can gather some statistics first and decide afterwards :) > In https://reviews.llvm.org/D53771#1294241, @JonasToth wrote: > >> LGTM. >> Please give other reviewers a chance to take a look at it too. > > > Thank you for the review! > I will keep this open for a few more days. np, i think until monday is enough. 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] D52984: [analyzer] Checker reviewer's checklist
NoQ added a comment. In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote: > Personally, I think it's detrimental to the community for subprojects to come > up with their own coding guidelines. My preference is for the static analyzer > to come more in line with the rest of the project (over time, organically) in > terms of style, terminology, diagnostic wording, etc. However, if the > consensus is that we want a separate coding standard, I think it should be > explicitly documented somewhere public and then maintained as part of the > project. I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid `addTransition()` hell - i guess we already have that, or how to register custom immutable maps, or how to implement checker dependencies or inter-checker APIs, or how much do we want to split modeling and checking into separate checkers, stuff like that. Comment at: www/analyzer/checker_dev_manual.html:719 + +User facing documentation is important for adoption! Make sure the check list updated +at the homepage of the analyzer. Also ensure that the description is good quality in aaron.ballman wrote: > xazax.hun wrote: > > Szelethus wrote: > > > xazax.hun wrote: > > > > Szelethus wrote: > > > > > Make sure the **checker** list **is** updated > > > > I think at some point we should decide if we prefer the term check or > > > > checker to refer to these things :) Clang Tidy clearly prefers check. > > > That is the distinction I'm aware of too: checkers in the Static > > > Analyzer, checks in clang-tidy. > > My understanding is the following: we want users to use the term `check`, > > since that is more widespread and used by other (non-clang) tools as well. > > The term `checker` is something like a historical artifact in the developer > > community of the static analyzer. But if this is not the case, I am happy > > to change the terminology. But I do want to have some input from rest of > > the community too :) > I grew up with the term "checker", but I feel like "check" may have won the > war. I don't have a strong opinion here though. We have the word "checker" all over the website, in option names, and, most importantly, in the "How to write a //checker// in 24 hours" video. I don't think we have much choice (: https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. In https://reviews.llvm.org/D52984#1294258, @NoQ wrote: > In https://reviews.llvm.org/D52984#1294233, @aaron.ballman wrote: > > > Personally, I think it's detrimental to the community for subprojects to > > come up with their own coding guidelines. My preference is for the static > > analyzer to come more in line with the rest of the project (over time, > > organically) in terms of style, terminology, diagnostic wording, etc. > > However, if the consensus is that we want a separate coding standard, I > > think it should be explicitly documented somewhere public and then > > maintained as part of the project. > > > I tihnk it's mostly conventions of using Analyzer-specific APIs, eg. avoid > `addTransition()` hell - i guess we already have that, or how to register > custom immutable maps, or how to implement checker dependencies or > inter-checker APIs, or how much do we want to split modeling and checking > into separate checkers, stuff like that. Indeed, I don't mean to change anything about the LLVM coding guideline, just add a couple Static Analyzer specific additional restrictions. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52984: [analyzer] Checker reviewer's checklist
Szelethus added a comment. Mainly my problem is that checker files can be large and hard to maneuver, some forward declare and document everything, some are a big bowl of spaghetti, and I think having a checklist of how it should be organized to keep uniformity and readability would be great. https://reviews.llvm.org/D52984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
JonasToth added a comment. > Done. Please also mark the inline comments as done. Otherwise its hard to follow if there are outstanding issues somewhere, especially if code moves around. https://reviews.llvm.org/D54349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346591 - [cxx_status] Update for San Diego motions.
Author: rsmith Date: Sat Nov 10 10:02:40 2018 New Revision: 346591 URL: http://llvm.org/viewvc/llvm-project?rev=346591&view=rev Log: [cxx_status] Update for San Diego motions. Modified: cfe/trunk/www/cxx_status.html Modified: cfe/trunk/www/cxx_status.html URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/www/cxx_status.html?rev=346591&r1=346590&r2=346591&view=diff == --- cfe/trunk/www/cxx_status.html (original) +++ cfe/trunk/www/cxx_status.html Sat Nov 10 10:02:40 2018 @@ -863,13 +863,19 @@ as the draft C++2a standard evolves. No - Concepts + Concepts http://wg21.link/p0734r0";>P0734R0 - No + No http://wg21.link/p0857r0";>P0857R0 + +http://wg21.link/p1084r2";>P1084R2 + + +http://wg21.link/p1141r2";>P1414R2 + Range-based for statements with initializer @@ -945,20 +951,32 @@ as the draft C++2a standard evolves. Clang 6 - Virtual function calls in constant expressions + Relaxations of constexpr restrictions http://wg21.link/p1064r0";>P1064R0 - No + No + +http://wg21.link/p1002r1";>P1002R1 + + +http://wg21.link/p1327r1";>P1327R1 + + +http://wg21.link/p1330r0";>P1330R0 + Prohibit aggregates with user-declared constructors http://wg21.link/p1008r1";>P1008R1 SVN - Contracts + Contracts http://wg21.link/p0542r5";>P0542R5 - No + No + +http://wg21.link/p1289r1";>P1289R1 + Feature test macros http://wg21.link/p0941r2";>P0941R2 @@ -969,6 +987,32 @@ as the draft C++2a standard evolves. http://wg21.link/p0892r2";>P0892R2 No + + + Signed integers are two's complement + http://wg21.link/p1236r1";>P1236R1 + No + + + char8_t + http://wg21.link/p0482r6";>P0482R6 + No + + + Immediate functions (consteval) + http://wg21.link/p1073r3";>P1073R3 + No + + + std::is_constant_evaluated + http://wg21.link/p0595r2";>P0595R2 + No + + + Nested inline namespaces + http://wg21.link/p1094r2";>P1094R2 + No + @@ -1050,7 +1094,7 @@ and library features that are not part o Superseded by P0734R0 - + [DRAFT TS] Coroutines https://isocpp.org/files/papers/N4663.pdf";>N4663 -fcoroutines-ts-stdlib=libc++ ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
Szelethus added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
This is a way too general name in my opinion. Either include comments that
describe it, or rename it (preferably both).
https://reviews.llvm.org/D54349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
erik.pilkington added a comment.
I have a few nits, but I think this looks about right.
I reduced this testcase with creduce, can you use the minimized version? Also,
I think it makes more sense to put the test into `test/CodeGenCXX` and verify
the `-emit-llvm-only` output is correct with FileCheck. You can check out
pretty much any test in that directory for an example of how to do this if you
don't know already.
class a {
public:
~a();
};
class logger_base {
a d;
};
class e : logger_base {};
__attribute((no_destroy)) e g;
Comment at: lib/CodeGen/CGDeclCXX.cpp:72
+ // attribute (D54344).
+ if (D.hasAttr())
+return;
You should do this with `D.isNoDestroy(CGF.getContext())`, I suspect that this
will still crash with `-fno-c++-static-destructors` without an attribute, and
that function checks for the existence of that flag. Can you add a test for
that too?
https://reviews.llvm.org/D54344
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
vmiklos updated this revision to Diff 173524.
https://reviews.llvm.org/D54349
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantPreprocessorCheck.cpp
clang-tidy/readability/RedundantPreprocessorCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-redundant-preprocessor.rst
test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
test/clang-tidy/readability-redundant-preprocessor.cpp
test/clang-tidy/readability-redundant-preprocessor.h
Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+ same condition. For example:
+
+.. code-block:: c++
+
+ #ifdef FOO
+ #ifdef FOO // inner ifdef is considered redundant
+ void f();
+ #endif
+ #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+ #ifndef FOO
+ #ifndef FOO // inner ifndef is considered redundant
+ void f();
+ #endif
+ #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+ readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
Detects usage of magic numbers, numbers that are used as literals instead of
introduced via constants or symbols.
+- New :doc:`readability-redundant-preprocessor
+ ` check.
+
+ Finds potentially redundant preprocessor directives.
+
- New :doc:`readability-uppercase-literal-suffix
` check.
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// This check flag
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
vmiklos added a comment. > What do you think about code like: > > #if FOO == 4 > #if FOO == 4 > #endif > #endif > > #if defined(FOO) > #if defined(FOO) > #endif > #endif > > #if !defined(FOO) > #if !defined(FOO) > #endif > #endif I looked at supporting these, but it's not clear to me how to get e.g. the 'FOO 4' string (the condition) from the `If()` callback. So far I only see how to get the start location of the condition and the whole range till the end of endif. Do you have a code pointer how to do this, please? (Or OK if I leave this for a follow-up patch?) > #if defined(FOO) > #if !defined(FOO) > #endif > #endif > > #if !defined(FOO) > #if defined(FOO) > #endif > #endif I expect handling these correctly is more complex -- I would prefer focusing on matching conditons in this patch, and get back to inverted conditions in a follow-up patch. Is that OK? If we handle inverted conditions, then handling `a && b` and `!a || !b` would make sense as well for example. > Please keep this list sorted alphabetically. Done. > This name is not particularly descriptive. This seems to be more like > `CheckMacroRedundancy` or something like that? Makes sense, done. > This comment should be re-flowed to fit the column width. Done. > What constitutes "redundancy"? A bit more exposition here would be useful. Hopefully "nested directives with the same condition" makes it easier to understand the intention and current scope. https://reviews.llvm.org/D54349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
vmiklos marked an inline comment as done.
vmiklos added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
Szelethus wrote:
> This is a way too general name in my opinion. Either include comments that
> describe it, or rename it (preferably both).
Renamed to `PreprocessorCondition`, hope it helps. :-)
https://reviews.llvm.org/D54349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
vmiklos updated this revision to Diff 173526.
vmiklos marked an inline comment as done.
https://reviews.llvm.org/D54349
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tidy/readability/RedundantPreprocessorCheck.cpp
clang-tidy/readability/RedundantPreprocessorCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-redundant-preprocessor.rst
test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
test/clang-tidy/readability-redundant-preprocessor.cpp
test/clang-tidy/readability-redundant-preprocessor.h
Index: test/clang-tidy/readability-redundant-preprocessor.h
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.h
@@ -0,0 +1,5 @@
+#ifndef FOO
+#ifndef FOO // this would warn, but not in a header
+void f();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -I %S
+
+// Positive testing.
+#ifndef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifndef; consider removing it [readability-redundant-preprocessor]
+#ifndef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifndef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#include "readability-redundant-preprocessor.h"
+
+#ifndef BAR
+void g();
+#endif
+
+#ifndef FOO
+#ifndef BAR
+void h();
+#endif
+#endif
Index: test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-preprocessor-ifdef.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s readability-redundant-preprocessor %t -- -- -DFOO
+
+// Positive testing.
+#ifdef FOO
+// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant ifdef; consider removing it [readability-redundant-preprocessor]
+#ifdef FOO
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous ifdef was here
+void f();
+#endif
+#endif
+
+// Negative testing.
+#ifdef BAR
+void g();
+#endif
+
+#ifdef FOO
+#ifdef BAR
+void h();
+#endif
+#endif
Index: docs/clang-tidy/checks/readability-redundant-preprocessor.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-preprocessor.rst
@@ -0,0 +1,29 @@
+.. title:: clang-tidy - readability-redundant-preprocessor
+
+readability-redundant-preprocessor
+==
+
+Finds potentially redundant preprocessor directives. At the moment the
+following cases are detected:
+
+* `#ifdef` .. `#endif` pairs which are nested inside an outer pair with the
+ same condition. For example:
+
+.. code-block:: c++
+
+ #ifdef FOO
+ #ifdef FOO // inner ifdef is considered redundant
+ void f();
+ #endif
+ #endif
+
+* Same for `#ifndef` .. `#endif` pairs. For example:
+
+.. code-block:: c++
+
+ #ifndef FOO
+ #ifndef FOO // inner ifndef is considered redundant
+ void f();
+ #endif
+ #endif
+
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -244,6 +244,7 @@
readability-redundant-declaration
readability-redundant-function-ptr-dereference
readability-redundant-member-init
+ readability-redundant-preprocessor
readability-redundant-smartptr-get
readability-redundant-string-cstr
readability-redundant-string-init
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -154,6 +154,11 @@
Detects usage of magic numbers, numbers that are used as literals instead of
introduced via constants or symbols.
+- New :doc:`readability-redundant-preprocessor
+ ` check.
+
+ Finds potentially redundant preprocessor directives.
+
- New :doc:`readability-uppercase-literal-suffix
` check.
Index: clang-tidy/readability/RedundantPreprocessorCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantPreprocessorCheck.h
@@ -0,0 +1,35 @@
+//===--- RedundantPreprocessorCheck.h - clang-tidy --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_REDUNDANTPREPROCESSORCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+name
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
Szelethus added inline comments.
Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:19-22
+struct Entry {
+ SourceLocation Loc;
+ std::string MacroName;
+};
vmiklos wrote:
> Szelethus wrote:
> > This is a way too general name in my opinion. Either include comments that
> > describe it, or rename it (preferably both).
> Renamed to `PreprocessorCondition`, hope it helps. :-)
I actually meant it for `Entry`, if you hover your mouse over an inline
comment, you can see which lines it applies to. Sorry for the confusing
communication :D
https://reviews.llvm.org/D54349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
bobsayshilol added a comment. Thanks! I don't have commit access to land this myself. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'
aaron.ballman added a comment. In https://reviews.llvm.org/D54349#1294325, @vmiklos wrote: > > What do you think about code like: > > > > #if FOO == 4 > > #if FOO == 4 > > #endif > > #endif > > > > #if defined(FOO) > > #if defined(FOO) > > #endif > > #endif > > > > #if !defined(FOO) > > #if !defined(FOO) > > #endif > > #endif > > I looked at supporting these, but it's not clear to me how to get e.g. the > 'FOO == 4' string (the condition) from the `If()` callback. So far I only see > how to get the start location of the condition and the whole range till the > end of endif. Do you have a code pointer how to do this, please? (Or OK if I > leave this for a follow-up patch?) I think it requires manual work. There's a FIXME in `PPCallbacks::If()` to pass a list/tree of tokens that would make implementing this reasonable. I'd say it's fine as a follow-up patch. >> #if defined(FOO) >> #if !defined(FOO) >> #endif >> #endif >> >> #if !defined(FOO) >> #if defined(FOO) >> #endif >> #endif > > I expect handling these correctly is more complex -- I would prefer focusing > on matching conditons in this patch, and get back to inverted conditions in a > follow-up patch. Is that OK? If we handle inverted conditions, then handling > `a && b` and `!a || !b` would make sense as well for example. I agree that it's more complex, but that's why I was asking for it -- I don't think the design you have currently will extend for these sort of cases, and I was hoping to cover more utility with the check to hopefully shake out those forward-looking design considerations. As it stands, I feel like this check is a bit too simplistic to have much value, but if you covered some of these other cases, it would alleviate that worry for me. https://reviews.llvm.org/D54349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libclc] r346597 - r600: Add datalayout to image builtin implementation
Author: jvesely Date: Sat Nov 10 13:43:40 2018 New Revision: 346597 URL: http://llvm.org/viewvc/llvm-project?rev=346597&view=rev Log: r600: Add datalayout to image builtin implementation Signed-off-by: Jan Vesely Reviewer: Aaron Watry Modified: libclc/trunk/r600/lib/image/get_image_attributes_impl.ll libclc/trunk/r600/lib/image/read_image_impl.ll libclc/trunk/r600/lib/image/write_image_impl.ll Modified: libclc/trunk/r600/lib/image/get_image_attributes_impl.ll URL: http://llvm.org/viewvc/llvm-project/libclc/trunk/r600/lib/image/get_image_attributes_impl.ll?rev=346597&r1=346596&r2=346597&view=diff == --- libclc/trunk/r600/lib/image/get_image_attributes_impl.ll (original) +++ libclc/trunk/r600/lib/image/get_image_attributes_impl.ll Sat Nov 10 13:43:40 2018 @@ -1,3 +1,5 @@ +target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64" + %opencl.image2d_t = type opaque %opencl.image3d_t = type opaque Modified: libclc/trunk/r600/lib/image/read_image_impl.ll URL: http://llvm.org/viewvc/llvm-project/libclc/trunk/r600/lib/image/read_image_impl.ll?rev=346597&r1=346596&r2=346597&view=diff == --- libclc/trunk/r600/lib/image/read_image_impl.ll (original) +++ libclc/trunk/r600/lib/image/read_image_impl.ll Sat Nov 10 13:43:40 2018 @@ -1,3 +1,5 @@ +target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64" + %opencl.image2d_t = type opaque declare <4 x float> @llvm.R600.tex(<4 x float>, i32, i32, i32, i32, i32, i32, Modified: libclc/trunk/r600/lib/image/write_image_impl.ll URL: http://llvm.org/viewvc/llvm-project/libclc/trunk/r600/lib/image/write_image_impl.ll?rev=346597&r1=346596&r2=346597&view=diff == --- libclc/trunk/r600/lib/image/write_image_impl.ll (original) +++ libclc/trunk/r600/lib/image/write_image_impl.ll Sat Nov 10 13:43:40 2018 @@ -1,3 +1,5 @@ +target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64" + %opencl.image2d_t = type opaque %opencl.image3d_t = type opaque ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment. I have the result after running the current version if the check. Found around 50 new issues were hidden by macros: https://cgit.freedesktop.org/libreoffice/core/commit/?id=afbfe42e63cdba1a18c292d7eb4875009b0f19c0 Also found some new false positives related to macros. The number of all false positives is around 38, which is still seems good to me. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
kristina updated this revision to Diff 173530.
kristina set the repository for this revision to rC Clang.
kristina added a comment.
@erik.pilkington Fantastic catch, I totally forgot about the global flag.
Revised, moved to CodeGenCXX, added a test to verify ctor/dtor is balanced
under normal circumstances and two tests to ensure ctor is not balanced with
either flag or attribute.
=> ninja check-clang-codegencxx
Testing Time: 7.68s
Expected Passes: 927
Expected Failures : 2
Unsupported Tests : 5
The attribute was actually missing tests for IR generation so thank you for
suggesting moving them there, I think this covers all cases for this bug
(balanced regression test without attr or flag, unbalanced test because of
attr, unbalanced test because of global flag).
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/CodeGenCXX/attr-no-destroy-d54344.cpp
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
-DNOATTR %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s
--check-prefix=CHECK-FLAG
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+ ~a();
+};
+class logger_base {
+ a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute.
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
/// static storage duration.
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
ConstantAddress Addr) {
+ // Honor __attribute__((no_destroy)) and bail instead of attempting
+ // to emit a reference to a possibly non-existant destructor, which
+ // in turn can cause a crash. This will result in a global constructor
+ // that isn't balanced out by a destructor call as intended by the
+ // attribute. This also checks for -fno-c++-static-destructors and
+ // bails even if the attribute is not present. (D54344)
+ if (D.isNoDestroy(CGF.getContext()))
+return;
+
CodeGenModule &CGM = CGF.CGM;
// FIXME: __attribute__((cleanup)) ?
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+ ~a();
+};
+class logger_base {
+ a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
kristina added a comment. In https://reviews.llvm.org/D53263#1294336, @bobsayshilol wrote: > Thanks! > I don't have commit access to land this myself. I can do it if you'd like, will be a moment though. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas added a comment.
I also tested on LLVm code.
The output is here:
https://gist.github.com/tzolnai/fe4f23031d3f9fdbdbf1ee38abda00a4
I found 362 warnings.
Around 95% of these warnings are similar to the next example:
/home/zolnai/lohome/llvm/lib/Support/Chrono.cpp:64:24: warning: loop variable
has narrower type 'unsigned int' than iteration's upper bound 'size_t' (aka
'unsigned long') [bugprone-too-small-loop-variable]
for (unsigned I = 0; I < Style.size(); ++I) {
Where the loop variable has an `unsigned int` type while in the loop condition
it is compared with a container size which has `size_t` type. The actualy size
method can be std::string::length() or array_lengthof() too.
An interesting catch related to a template function:
/home/zolnai/lohome/llvm/tools/clang/lib/CodeGen/CGNonTrivialStruct.cpp:310:24:
warning: loop variable has narrower type 'unsigned int' than iteration's upper
bound 'size_t' (aka 'unsigned long') [bugprone-too-small-loop-variable]
for (unsigned I = 0; I < N; ++I)
Where N is a template value with `size_t` type. If the container function is
instantiated with a "large" value I expect the function won't work. I guess it
never actually happens.
An other catch inside a macro:
/home/zolnai/lohome/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp:157:5:
warning: loop variable has narrower type 'uint32_t' (aka 'unsigned int') than
iteration's upper bound 'std::vector::size_type' (aka 'unsigned long')
[bugprone-too-small-loop-variable]
IMPLEMENT_VECTOR_INTEGER_ICMP(ne,Ty);
^
/home/zolnai/lohome/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp:123:24:
note: expanded from macro 'IMPLEMENT_VECTOR_INTEGER_ICMP'
for( uint32_t _i=0;_ihttps://reviews.llvm.org/D53974
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some small commenting nits. However, I leave it to @erik.pilkington to make the final sign-off. Comment at: lib/CodeGen/CGDeclCXX.cpp:68 + // Honor __attribute__((no_destroy)) and bail instead of attempting + // to emit a reference to a possibly non-existant destructor, which + // in turn can cause a crash. This will result in a global constructor non-existant -> nonexistent Comment at: lib/CodeGen/CGDeclCXX.cpp:72 + // attribute. This also checks for -fno-c++-static-destructors and + // bails even if the attribute is not present. (D54344) + if (D.isNoDestroy(CGF.getContext())) Drop the (D54344) from the comment. Repository: rC Clang https://reviews.llvm.org/D54344 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54379: Add Hurd support
krytarowski added inline comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:136 + + // Add an include of '/include' directly. This isn't provided by default by + // system GCCs, but is often used with cross-compiling GCCs, and harmless to Is this some hurd standard or personal taste? https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment. Thank you for checking these two projects! In https://reviews.llvm.org/D53974#1294375, @ztamas wrote: > I have the result after running the current version of the check on > LibreOffice. > > Found around 50 new issues were hidden by macros: > > https://cgit.freedesktop.org/libreoffice/core/commit/?id=afbfe42e63cdba1a18c292d7eb4875009b0f19c0 > > Also found some new false positives related to macros. The number of all > false positives is around 38, which is still seems good to me. I would be very interested why these false positives are created. Is there a way to avoid them and maybe it makes sense to already add `FIXME` at the possible places the check needs improvements. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
bobsayshilol added a comment. In https://reviews.llvm.org/D53263#1294383, @kristina wrote: > I can do it if you'd like, will be a moment though. Thanks and much appreciated! https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.
kristina updated this revision to Diff 173534.
kristina added a comment.
Revised to address nitpicks.
Repository:
rC Clang
https://reviews.llvm.org/D54344
Files:
lib/CodeGen/CGDeclCXX.cpp
test/CodeGenCXX/attr-no-destroy-d54344.cpp
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
-DNOATTR %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
%s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu
-DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s
--check-prefix=CHECK-FLAG
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+ ~a();
+};
+class logger_base {
+ a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute.
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -64,6 +64,15 @@
/// static storage duration.
static void EmitDeclDestroy(CodeGenFunction &CGF, const VarDecl &D,
ConstantAddress Addr) {
+ // Honor __attribute__((no_destroy)) and bail instead of attempting
+ // to emit a reference to a possibly nonexistent destructor, which
+ // in turn can cause a crash. This will result in a global constructor
+ // that isn't balanced out by a destructor call as intended by the
+ // attribute. This also checks for -fno-c++-static-destructors and
+ // bails even if the attribute is not present.
+ if (D.isNoDestroy(CGF.getContext()))
+return;
+
CodeGenModule &CGM = CGF.CGM;
// FIXME: __attribute__((cleanup)) ?
Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp
===
--- test/CodeGenCXX/attr-no-destroy-d54344.cpp
+++ test/CodeGenCXX/attr-no-destroy-d54344.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR %s -o - | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu %s -o - | FileCheck %s --check-prefix=CHECK-ATTR
+// RUN: %clang_cc1 -std=c++2a -emit-llvm -O0 -triple x86_64-unknown-linux-gnu -DNOATTR -fno-c++-static-destructors %s -o - | FileCheck %s --check-prefix=CHECK-FLAG
+// REQUIRES: asserts
+
+// Regression test for D54344. Class with no user-defined destructor
+// that has an inherited member that has a non-trivial destructor
+// and a non-default constructor will attempt to emit a destructor
+// despite being marked as __attribute((no_destroy)) in which case
+// it would trigger an assertion due to an incorrect assumption.
+// This test requires asserts to reliably work as without the crash
+// it generates working but semantically incorrect code.
+
+class a {
+public:
+ ~a();
+};
+class logger_base {
+ a d;
+};
+class e : logger_base {};
+#ifndef NOATTR
+__attribute((no_destroy))
+#endif
+e g;
+
+// In the absence of the attribute and flag, both ctor and dtor should
+// be emitted, check for that.
+// CHECK: @__cxx_global_var_init
+// CHECK: @__cxa_atexit
+
+// When attribute is enabled, the constructor should not be balanced
+// by a destructor. Make sure we have the ctor but not the dtor
+// registration.
+// CHECK-ATTR: @__cxx_global_var_init
+// CHECK-ATTR-NOT: @__cxa_atexit
+
+// Same scenario except with global flag (-fno-c++-static-destructors)
+// supressing it instead of the attribute.
+// CHECK-FLAG: @__cxx_global_var_init
+// CHECK-FLAG-NOT: @__cxa_atexit
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCX
[PATCH] D54379: Add Hurd support
sthibaul added inline comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:136 + + // Add an include of '/include' directly. This isn't provided by default by + // system GCCs, but is often used with cross-compiling GCCs, and harmless to krytarowski wrote: > Is this some hurd standard or personal taste? I copied this from the Linux.cpp file. Actually it happens to be a standard in the pure GNU system which does not define a /usr. Debian GNU/Hurd eventually migrated to having a real /usr just like other Debian ports to keep things coherent, but the GNU system is supposed to have prefix=/ https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
kristina added a comment. In https://reviews.llvm.org/D53263#1294412, @bobsayshilol wrote: > In https://reviews.llvm.org/D53263#1294383, @kristina wrote: > > > I can do it if you'd like, will be a moment though. > > > Thanks and much appreciated! Huge apologies, it seems I can't get this to patch cleanly against my fork and therefore can't test it before committing, which is something I generally always do. I'll leave it to someone else. Again, huge apologies, hopefully you won't have to wait too long. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
JDevlieghere added a comment. The patch applies for me but has quite a few style violations. I'll fix those up before landing it. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346601 - Pass the function type instead of the return type to FunctionDecl::Create
Author: jdevlieghere
Date: Sat Nov 10 16:56:15 2018
New Revision: 346601
URL: http://llvm.org/viewvc/llvm-project?rev=346601&view=rev
Log:
Pass the function type instead of the return type to FunctionDecl::Create
Fix places where the return type of a FunctionDecl was being used in
place of the function type
FunctionDecl::Create() takes as its T parameter the type of function
that should be created, not the return type. Passing in the return type
looks to have been copypasta'd around a bit, but the number of correct
usages outweighs the incorrect ones so I've opted for keeping what T is
the same and fixing up the call sites instead.
This fixes a crash in Clang when attempting to compile the following
snippet of code with -fblocks -fsanitize=function -x objective-c++ (my
original repro case):
void g(void(^)());
void f()
{
__block int a = 0;
g(^(){ a++; });
}
as well as the following which only requires -fsanitize=function -x c++:
void f(char * buf)
{
__builtin_os_log_format(buf, "");
}
Patch by: Ben (bobsayshilol)
Differential revision: https://reviews.llvm.org/D53263
Added:
cfe/trunk/test/CodeGenObjCXX/crash-function-type.mm
Modified:
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/lib/CodeGen/CGBlocks.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
cfe/trunk/lib/CodeGen/CGObjC.cpp
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
Modified: cfe/trunk/lib/AST/Decl.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=346601&r1=346600&r2=346601&view=diff
==
--- cfe/trunk/lib/AST/Decl.cpp (original)
+++ cfe/trunk/lib/AST/Decl.cpp Sat Nov 10 16:56:15 2018
@@ -2652,6 +2652,7 @@ FunctionDecl::FunctionDecl(Kind DK, ASTC
StartLoc),
DeclContext(DK), redeclarable_base(C), ODRHash(0),
EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
+ assert(T.isNull() || T->isFunctionType());
setStorageClass(S);
setInlineSpecified(isInlineSpecified);
setExplicitSpecified(false);
Modified: cfe/trunk/lib/CodeGen/CGBlocks.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBlocks.cpp?rev=346601&r1=346600&r2=346601&view=diff
==
--- cfe/trunk/lib/CodeGen/CGBlocks.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBlocks.cpp Sat Nov 10 16:56:15 2018
@@ -2008,16 +2008,16 @@ CodeGenFunction::GenerateCopyHelperFunct
ASTContext &C = getContext();
+ QualType ReturnTy = C.VoidTy;
+
FunctionArgList args;
- ImplicitParamDecl DstDecl(getContext(), C.VoidPtrTy,
-ImplicitParamDecl::Other);
+ ImplicitParamDecl DstDecl(C, C.VoidPtrTy, ImplicitParamDecl::Other);
args.push_back(&DstDecl);
- ImplicitParamDecl SrcDecl(getContext(), C.VoidPtrTy,
-ImplicitParamDecl::Other);
+ ImplicitParamDecl SrcDecl(C, C.VoidPtrTy, ImplicitParamDecl::Other);
args.push_back(&SrcDecl);
const CGFunctionInfo &FI =
-CGM.getTypes().arrangeBuiltinFunctionDeclaration(C.VoidTy, args);
+ CGM.getTypes().arrangeBuiltinFunctionDeclaration(ReturnTy, args);
// FIXME: it would be nice if these were mergeable with things with
// identical semantics.
@@ -2027,20 +2027,20 @@ CodeGenFunction::GenerateCopyHelperFunct
llvm::Function::Create(LTy, llvm::GlobalValue::LinkOnceODRLinkage,
FuncName, &CGM.getModule());
- IdentifierInfo *II
-= &CGM.getContext().Idents.get(FuncName);
+ IdentifierInfo *II = &C.Idents.get(FuncName);
- FunctionDecl *FD = FunctionDecl::Create(C,
- C.getTranslationUnitDecl(),
- SourceLocation(),
- SourceLocation(), II, C.VoidTy,
- nullptr, SC_Static,
- false,
- false);
+ SmallVector ArgTys;
+ ArgTys.push_back(C.VoidPtrTy);
+ ArgTys.push_back(C.VoidPtrTy);
+ QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
+ FunctionDecl *FD = FunctionDecl::Create(
+ C, C.getTranslationUnitDecl(), SourceLocation(), SourceLocation(), II,
+ FunctionTy, nullptr, SC_Static, false, false);
setBlockHelperAttributesVisibility(blockInfo.CapturesNonExternalType, Fn, FI,
CGM);
- StartFunction(FD, C.VoidTy, Fn, FI, args);
+ StartFunction(FD, ReturnTy, Fn, FI, args);
ApplyDebugLocation NL{*this, blockInfo.getBlockExpr()->getBeginLoc()};
llvm::Type *structPtrTy = blockInfo.StructureType->getPointerTo();
@@ -2201,13 +2201,14 @@ CodeGenFunction::GenerateDestroyHelperFu
ASTContext &C = getContext();
+ QualType
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
kristina added a comment. In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote: > The patch applies for me but has quite a few style violations. I'll fix those > up before landing it. Thank you and sorry for the trouble, my fork seems to have enough modifications to some of these files to confuse `patch` and getting an untainted checkout and integrating it for a single build/test run would be troublesome to undo. Repository: rL LLVM https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346601: Pass the function type instead of the return type to
FunctionDecl::Create (authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53263?vs=170277&id=173542#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53263
Files:
cfe/trunk/lib/AST/Decl.cpp
cfe/trunk/lib/CodeGen/CGBlocks.cpp
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/lib/CodeGen/CGNonTrivialStruct.cpp
cfe/trunk/lib/CodeGen/CGObjC.cpp
cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp
cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
cfe/trunk/test/CodeGenObjCXX/crash-function-type.mm
Index: cfe/trunk/lib/AST/Decl.cpp
===
--- cfe/trunk/lib/AST/Decl.cpp
+++ cfe/trunk/lib/AST/Decl.cpp
@@ -2652,6 +2652,7 @@
StartLoc),
DeclContext(DK), redeclarable_base(C), ODRHash(0),
EndRangeLoc(NameInfo.getEndLoc()), DNLoc(NameInfo.getInfo()) {
+ assert(T.isNull() || T->isFunctionType());
setStorageClass(S);
setInlineSpecified(isInlineSpecified);
setExplicitSpecified(false);
Index: cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
===
--- cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
+++ cfe/trunk/lib/Frontend/Rewrite/RewriteModernObjC.cpp
@@ -3099,10 +3099,9 @@
SmallVectorImpl &MsgExprs,
ObjCMethodDecl *Method) {
// Now do the "normal" pointer to function cast.
- QualType castType = getSimpleFunctionType(returnType, ArgTypes,
-Method ? Method->isVariadic()
- : false);
- castType = Context->getPointerType(castType);
+ QualType FuncType = getSimpleFunctionType(
+ returnType, ArgTypes, Method ? Method->isVariadic() : false);
+ QualType castType = Context->getPointerType(FuncType);
// build type for containing the objc_msgSend_stret object.
static unsigned stretCount=0;
@@ -3176,9 +3175,9 @@
// AST for __Stretn(receiver, args).s;
IdentifierInfo *ID = &Context->Idents.get(name);
- FunctionDecl *FD = FunctionDecl::Create(*Context, TUDecl, SourceLocation(),
- SourceLocation(), ID, castType,
- nullptr, SC_Extern, false, false);
+ FunctionDecl *FD =
+ FunctionDecl::Create(*Context, TUDecl, SourceLocation(), SourceLocation(),
+ ID, FuncType, nullptr, SC_Extern, false, false);
DeclRefExpr *DRE = new (Context) DeclRefExpr(FD, false, castType, VK_RValue,
SourceLocation());
CallExpr *STCE = new (Context) CallExpr(*Context, DRE, MsgExprs,
Index: cfe/trunk/lib/CodeGen/CGObjC.cpp
===
--- cfe/trunk/lib/CodeGen/CGObjC.cpp
+++ cfe/trunk/lib/CodeGen/CGObjC.cpp
@@ -3249,29 +3249,32 @@
ASTContext &C = getContext();
IdentifierInfo *II
= &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
- FunctionDecl *FD = FunctionDecl::Create(C,
- C.getTranslationUnitDecl(),
- SourceLocation(),
- SourceLocation(), II, C.VoidTy,
- nullptr, SC_Static,
- false,
- false);
+ QualType ReturnTy = C.VoidTy;
QualType DestTy = C.getPointerType(Ty);
QualType SrcTy = Ty;
SrcTy.addConst();
SrcTy = C.getPointerType(SrcTy);
+ SmallVector ArgTys;
+ ArgTys.push_back(DestTy);
+ ArgTys.push_back(SrcTy);
+ QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
+ FunctionDecl *FD = FunctionDecl::Create(
+ C, C.getTranslationUnitDecl(), SourceLocation(), SourceLocation(), II,
+ FunctionTy, nullptr, SC_Static, false, false);
+
FunctionArgList args;
- ImplicitParamDecl DstDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
-DestTy, ImplicitParamDecl::Other);
+ ImplicitParamDecl DstDecl(C, FD, SourceLocation(), /*Id=*/nullptr, DestTy,
+ImplicitParamDecl::Other);
args.push_back(&DstDecl);
- ImplicitParamDecl SrcDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
-SrcTy, ImplicitParamDecl::Other);
+ ImplicitParamDecl SrcDecl(C, FD, SourceLocation(), /*Id=*/nullptr, SrcTy,
+ImplicitParamDecl::Other);
args.push_back(&SrcDecl);
const CGFunctionInfo &FI =
-CGM.getTypes().arrangeBuiltinFunctionDeclaration(C.VoidTy, args);
+
[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type
JDevlieghere added a comment. In https://reviews.llvm.org/D53263#1294497, @kristina wrote: > In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote: > > > The patch applies for me but has quite a few style violations. I'll fix > > those up before landing it. > > > Thank you and sorry for the trouble, my fork seems to have enough > modifications to some of these files to confuse `patch` and getting an > untainted checkout and integrating it for a single build/test run would be > troublesome to undo. No worries! I usually have a pretty up-to-date checkout around (I have cronjob that pulls and builds overnight) which comes in handy in situations like this. Repository: rL LLVM https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check
hwright updated this revision to Diff 173543.
hwright marked 16 inline comments as done.
hwright added a comment.
Addressed reviewer feedback.
https://reviews.llvm.org/D54246
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/DurationFactoryScaleCheck.cpp
clang-tidy/abseil/DurationFactoryScaleCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-duration-factory-scale.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-duration-factory-scale.cpp
Index: test/clang-tidy/abseil-duration-factory-scale.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-factory-scale.cpp
@@ -0,0 +1,110 @@
+// RUN: %check_clang_tidy %s abseil-duration-factory-scale %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+ Duration NAME(float n); \
+ Duration NAME(double n);\
+ template\
+ Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+} // namespace absl
+
+void ScaleTest() {
+ absl::Duration d;
+
+ // Zeroes
+ d = absl::Hours(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Minutes(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Seconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Milliseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Microseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Nanoseconds(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+ d = absl::Seconds(0.0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use ZeroDuration() for zero-length time intervals [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::ZeroDuration();
+
+ // Fold seconds into minutes
+ d = absl::Seconds(30 * 60);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+ d = absl::Seconds(60 * 30);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+
+ // Try a few more exotic multiplications
+ d = absl::Seconds(60 * 30 * 60);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(60 * 30);
+ d = absl::Seconds(1e-3 * 30);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Milliseconds(30);
+ d = absl::Milliseconds(30 * 1000);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Seconds(30);
+ d = absl::Milliseconds(30 * 0.001);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Microseconds(30);
+
+ // Division
+ d = absl::Hours(30 / 60.);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Minutes(30);
+ d = absl::Seconds(30 / 1000.);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Milliseconds(30);
+ d = absl::Milliseconds(30 / 1e3);
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: internal duration scaling can be removed [abseil-duration-factory-scale]
+ // CHECK-FIXES: absl::Microseconds(30);
+
+ // None of these should trig
[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check
hwright added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:74 + case DurationScale::Minutes: +if (Multiplier == 60.0) + return DurationScale::Hours; hokein wrote: > we are comparing with floats, I think we should use something > `AlmostEquals(Multiplier, 60.0)`. I can't find AlmostEquals elsewhere; could you point me to it so I can investigate it's use? But I am also curious why you think we should use this. We are checking for exact values. 60.0 is representable exactly as a `double`, and even values which aren't (e.g., 1e-3) will have the same representation in this function as they do in the code being transformed, so equality is still appropriate. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:202 + // Don't try and replace things inside of macro definitions. + if (InsideMacroDefinition(Result, Call->getSourceRange())) +return; hokein wrote: > isn't `Call->getExprLoc().isMacroID()` enough? Thanks! I've also added tests to verify this. https://reviews.llvm.org/D54246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜
stephanemoore updated this revision to Diff 173544.
stephanemoore marked 9 inline comments as done.
stephanemoore added a comment.
Updated with the following changes:
• Elided conditional braces to comply with LLVM style.
• Converted conditional loop break to loop condition in generateFixItHint.
• Fixed Objective-C language option check.
• Removed unnecessary assertions.
• Use `MatchedDecl` directly for formatting the diagnostic message.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51575
Files:
clang-tidy/google/CMakeLists.txt
clang-tidy/google/FunctionNamingCheck.cpp
clang-tidy/google/FunctionNamingCheck.h
clang-tidy/google/GoogleTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/google-objc-function-naming.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/google-objc-function-naming.m
Index: test/clang-tidy/google-objc-function-naming.m
===
--- /dev/null
+++ test/clang-tidy/google-objc-function-naming.m
@@ -0,0 +1,52 @@
+// RUN: %check_clang_tidy %s google-objc-function-naming %t
+
+typedef _Bool bool;
+
+static bool ispositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'ispositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool Ispositive(int a) { return a > 0; }
+
+static bool is_positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'is_positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool isPositive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'isPositive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool Is_Positive(int a) { return a > 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function name 'Is_Positive' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static bool IsPositive(int a) { return a > 0; }
+
+static bool IsPositive(int a) { return a > 0; }
+
+bool ispalindrome(const char *str);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ispalindrome' not using function naming conventions described by Google Objective-C style guide
+
+static const char *md5(const char *str) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: function name 'md5' not using function naming conventions described by Google Objective-C style guide
+// CHECK-FIXES: static const char *Md5(const char *str) { return 0; }
+
+static const char *MD5(const char *str) { return 0; }
+
+static const char *URL(void) { return "https://clang.llvm.org/";; }
+
+static const char *DEFURL(void) { return "https://clang.llvm.org/";; }
+
+static const char *DEFFooURL(void) { return "https://clang.llvm.org/";; }
+
+static const char *StringFromNSString(id str) { return ""; }
+
+void ABLog_String(const char *str);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'ABLog_String' not using function naming conventions described by Google Objective-C style guide
+
+void ABLogString(const char *str);
+
+bool IsPrime(int a);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function name 'IsPrime' not using function naming conventions described by Google Objective-C style guide
+
+const char *ABURL(void) { return "https://clang.llvm.org/";; }
+
+const char *ABFooURL(void) { return "https://clang.llvm.org/";; }
+
+int main(int argc, const char **argv) { return 0; }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -122,6 +122,7 @@
google-explicit-constructor
google-global-names-in-headers
google-objc-avoid-throwing-exception
+ google-objc-function-naming
google-objc-global-variable-declaration
google-readability-braces-around-statements (redirects to readability-braces-around-statements)
google-readability-casting
Index: docs/clang-tidy/checks/google-objc-function-naming.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/google-objc-function-naming.rst
@@ -0,0 +1,27 @@
+.. title:: clang-tidy - google-objc-function-naming
+
+google-objc-function-naming
+===
+
+Finds function declarations in Objective-C files that do not follow the pattern
+described in the Google Objective-C Style Guide.
+
+The corresponding style guide rule can be found here:
+https://google.github.io/styleguide/objcguide.html#function-names
+
+All function names should be in upper camel case. Functions whose storage class
+is not static should have an appropriate prefix.
+
+The following code sample does not follow this pattern:
+
+.. code-block
[PATCH] D54319: clang-cl: Add documentation for /Zc:dllexportInlines-
takuto.ikuta accepted this revision. takuto.ikuta added a comment. This revision is now accepted and ready to land. Seems good document! https://reviews.llvm.org/D54319 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
