[PATCH] D44865: [libc++] Implement P0608R1 - A sane variant converting constructor
lichray created this revision.
lichray added reviewers: EricWF, mpark, mclow.lists.
Herald added a subscriber: christof.
"Boolshit shall not prevail," LEWG says.
References:
http://wg21.link/P0608R1
Repository:
rCXX libc++
https://reviews.llvm.org/D44865
Files:
include/variant
test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
Index: test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
===
--- test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
+++ test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include "test_convertible.hpp"
#include "test_macros.h"
@@ -53,7 +54,7 @@
void test_T_ctor_sfinae() {
{
-using V = std::variant;
+using V = std::variant;
static_assert(!std::is_constructible::value, "ambiguous");
}
{
@@ -66,6 +67,16 @@
"no matching constructor");
}
{
+using V = std::variant;
+static_assert(!std::is_constructible::value,
+ "no matching constructor");
+ }
+ {
+using V = std::variant, bool>;
+static_assert(!std::is_constructible>::value,
+ "no explicit bool in constructor");
+ }
+ {
using V = std::variant;
static_assert(
!std::is_constructible>::value,
@@ -99,6 +110,21 @@
static_assert(v.index() == 1, "");
static_assert(std::get<1>(v) == 42, "");
}
+ {
+constexpr std::variant v(42);
+static_assert(v.index() == 1, "");
+static_assert(std::get<1>(v) == 42, "");
+ }
+ {
+std::variant v = "foo";
+assert(v.index() == 0);
+assert(std::get<0>(v) == "foo");
+ }
+ {
+std::variant> v = nullptr;
+assert(v.index() == 1);
+assert(std::get<1>(v) == nullptr);
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
Index: test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
===
--- test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
+++ test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include "test_macros.h"
#include "variant_test_helpers.hpp"
@@ -128,7 +129,7 @@
void test_T_assignment_sfinae() {
{
-using V = std::variant;
+using V = std::variant;
static_assert(!std::is_assignable::value, "ambiguous");
}
{
@@ -139,6 +140,15 @@
using V = std::variant;
static_assert(!std::is_assignable::value, "no matching operator=");
}
+ {
+using V = std::variant;
+static_assert(!std::is_assignable::value, "no matching operator=");
+ }
+ {
+using V = std::variant, bool>;
+static_assert(!std::is_assignable>::value,
+ "no explicit bool in operator=");
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
@@ -167,6 +177,33 @@
assert(v.index() == 1);
assert(std::get<1>(v) == 43);
}
+ {
+std::variant v;
+v = 42;
+assert(v.index() == 1);
+assert(std::get<1>(v) == 42);
+v = 43u;
+assert(v.index() == 0);
+assert(std::get<0>(v) == 43);
+ }
+ {
+std::variant v = true;
+v = std::false_type();
+assert(v.index() == 1);
+assert(std::get<1>(v) == false);
+v = "bar";
+assert(v.index() == 0);
+assert(std::get<0>(v) == "bar");
+ }
+ {
+std::variant> v;
+v = nullptr;
+assert(v.index() == 1);
+assert(std::get<1>(v) == nullptr);
+v = std::true_type();
+assert(v.index() == 0);
+assert(std::get<0>(v));
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
Index: include/variant
===
--- include/variant
+++ include/variant
@@ -1092,16 +1092,42 @@
struct __overload;
template <>
-struct __overload<> { void operator()() const; };
+struct __overload<>
+{
+static void test();
+};
template
-struct __overload<_Tp, _Types...> : __overload<_Types...> {
- using __overload<_Types...>::operator();
- __identity<_Tp> operator()(_Tp) const;
+struct __overload<_Tp, _Types...> : __overload<_Types...>
+{
+using __overload<_Types...>::test;
+
+template
+static auto test(_Tp, _Up&& __t) -> decltype(_Tp{__t}, __identity<_Tp>());
};
+#define _LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool_type)\
+template\
+struct __overload : __overload<_Types...> \
+{\
+using __overload<_Types...>::test; \
+ \
+template >
[PATCH] D44865: [libc++] Implement P0608R1 - A sane variant converting constructor
lichray updated this revision to Diff 139704.
lichray added a comment.
Use less traits
Repository:
rCXX libc++
https://reviews.llvm.org/D44865
Files:
include/variant
test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
Index: test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
===
--- test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
+++ test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include "test_convertible.hpp"
#include "test_macros.h"
@@ -53,7 +54,7 @@
void test_T_ctor_sfinae() {
{
-using V = std::variant;
+using V = std::variant;
static_assert(!std::is_constructible::value, "ambiguous");
}
{
@@ -66,6 +67,16 @@
"no matching constructor");
}
{
+using V = std::variant;
+static_assert(!std::is_constructible::value,
+ "no matching constructor");
+ }
+ {
+using V = std::variant, bool>;
+static_assert(!std::is_constructible>::value,
+ "no explicit bool in constructor");
+ }
+ {
using V = std::variant;
static_assert(
!std::is_constructible>::value,
@@ -99,6 +110,21 @@
static_assert(v.index() == 1, "");
static_assert(std::get<1>(v) == 42, "");
}
+ {
+constexpr std::variant v(42);
+static_assert(v.index() == 1, "");
+static_assert(std::get<1>(v) == 42, "");
+ }
+ {
+std::variant v = "foo";
+assert(v.index() == 0);
+assert(std::get<0>(v) == "foo");
+ }
+ {
+std::variant> v = nullptr;
+assert(v.index() == 1);
+assert(std::get<1>(v) == nullptr);
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
Index: test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
===
--- test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
+++ test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include "test_macros.h"
#include "variant_test_helpers.hpp"
@@ -128,7 +129,7 @@
void test_T_assignment_sfinae() {
{
-using V = std::variant;
+using V = std::variant;
static_assert(!std::is_assignable::value, "ambiguous");
}
{
@@ -139,6 +140,15 @@
using V = std::variant;
static_assert(!std::is_assignable::value, "no matching operator=");
}
+ {
+using V = std::variant;
+static_assert(!std::is_assignable::value, "no matching operator=");
+ }
+ {
+using V = std::variant, bool>;
+static_assert(!std::is_assignable>::value,
+ "no explicit bool in operator=");
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
@@ -167,6 +177,33 @@
assert(v.index() == 1);
assert(std::get<1>(v) == 43);
}
+ {
+std::variant v;
+v = 42;
+assert(v.index() == 1);
+assert(std::get<1>(v) == 42);
+v = 43u;
+assert(v.index() == 0);
+assert(std::get<0>(v) == 43);
+ }
+ {
+std::variant v = true;
+v = std::false_type();
+assert(v.index() == 1);
+assert(std::get<1>(v) == false);
+v = "bar";
+assert(v.index() == 0);
+assert(std::get<0>(v) == "bar");
+ }
+ {
+std::variant> v;
+v = nullptr;
+assert(v.index() == 1);
+assert(std::get<1>(v) == nullptr);
+v = std::true_type();
+assert(v.index() == 0);
+assert(std::get<0>(v));
+ }
#if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
{
using V = std::variant;
Index: include/variant
===
--- include/variant
+++ include/variant
@@ -1092,16 +1092,45 @@
struct __overload;
template <>
-struct __overload<> { void operator()() const; };
+struct __overload<>
+{
+static void test();
+};
template
-struct __overload<_Tp, _Types...> : __overload<_Types...> {
- using __overload<_Types...>::operator();
- __identity<_Tp> operator()(_Tp) const;
+struct __overload<_Tp, _Types...> : __overload<_Types...>
+{
+using __overload<_Types...>::test;
+
+template
+static auto test(_Tp, _Up&& __t) -> decltype(_Tp{__t}, __identity<_Tp>());
};
+#define _LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool_type)\
+template\
+struct __overload : __overload<_Types...> \
+{\
+using __overload<_Types...>::test; \
+ \
+template > \
+static auto test(bool, _Up&&, int _Ap::* = 0)\
+-> __identity;
[clang-tools-extra] r328418 - [clang-tidy] Enable Python 3 support for add_new_check.py
Author: jbcoe
Date: Sat Mar 24 03:49:17 2018
New Revision: 328418
URL: http://llvm.org/viewvc/llvm-project?rev=328418&view=rev
Log:
[clang-tidy] Enable Python 3 support for add_new_check.py
Summary: In Python 3, filters are lazily evaluated and strings are not bytes.
Reviewers: ilya-biryukov
Reviewed By: ilya-biryukov
Subscribers: xazax.hun, cfe-commits
Differential Revision: https://reviews.llvm.org/D44217
Modified:
clang-tools-extra/trunk/clang-tidy/add_new_check.py
Modified: clang-tools-extra/trunk/clang-tidy/add_new_check.py
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/add_new_check.py?rev=328418&r1=328417&r2=328418&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/add_new_check.py (original)
+++ clang-tools-extra/trunk/clang-tidy/add_new_check.py Sat Mar 24 03:49:17 2018
@@ -9,12 +9,13 @@
#
#======#
+from __future__ import print_function
+
import argparse
import os
import re
import sys
-
# Adapts the module's CMakelist file. Returns 'True' if it could add a new
entry
# and 'False' if the entry already existed.
def adapt_cmake(module_path, check_name_camel):
@@ -30,7 +31,7 @@ def adapt_cmake(module_path, check_name_
return False
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
cpp_found = False
file_added = False
for line in lines:
@@ -50,7 +51,7 @@ def write_header(module_path, module, ch
check_name_dashes = module + '-' + check_name
filename = os.path.join(module_path, check_name_camel) + '.h'
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
header_guard = ('LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_' + module.upper() + '_'
+ check_name_camel.upper() + '_H')
f.write('//===--- ')
@@ -103,7 +104,7 @@ public:
def write_implementation(module_path, module, check_name_camel):
filename = os.path.join(module_path, check_name_camel) + '.cpp'
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write('//===--- ')
f.write(os.path.basename(filename))
f.write(' - clang-tidy')
@@ -152,14 +153,15 @@ void %(check_name)s::check(const MatchFi
# Modifies the module to include the new check.
def adapt_module(module_path, module, check_name, check_name_camel):
- modulecpp = filter(lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
- os.listdir(module_path))[0]
+ modulecpp = list(filter(
+ lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
+ os.listdir(module_path)))[0]
filename = os.path.join(module_path, modulecpp)
with open(filename, 'r') as f:
lines = f.readlines()
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
header_added = False
header_found = False
check_added = False
@@ -199,7 +201,7 @@ def add_release_notes(module_path, modul
lines = f.readlines()
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
note_added = False
header_found = False
@@ -227,7 +229,7 @@ def write_test(module_path, module, chec
filename = os.path.normpath(os.path.join(module_path,
'../../test/clang-tidy',
check_name_dashes + '.' +
test_extension))
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write("""// RUN: %%check_clang_tidy %%s %(check_name_dashes)s %%t
// FIXME: Add something that triggers the check here.
@@ -251,8 +253,8 @@ def update_checks_list(clang_tidy_path):
filename = os.path.normpath(os.path.join(docs_dir, 'list.rst'))
with open(filename, 'r') as f:
lines = f.readlines()
- doc_files = filter(lambda s: s.endswith('.rst') and s != 'list.rst',
- os.listdir(docs_dir))
+ doc_files = list(filter(lambda s: s.endswith('.rst') and s != 'list.rst',
+ os.listdir(docs_dir)))
doc_files.sort()
def format_link(doc_file):
@@ -275,7 +277,7 @@ def update_checks_list(clang_tidy_path):
checks = map(format_link, doc_files)
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
for line in lines:
f.write(line)
if line.startswith('.. toctree::'):
@@ -289,7 +291,7 @@ def write_docs(module_path, module, chec
filename = os.path.normpath(os.path.join(
module_path, '../../docs/clang-tidy/checks/', check_name_dashes +
'.rst'))
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write(""".. title:: clang-tidy - %(check_name_dashes)s
%(check_name_dashes)s
@@ -333,7 +335,7 @@ def m
[PATCH] D44217: [clang-tidy] Enable Python 3 support for add_new_check.py
This revision was automatically updated to reflect the committed changes.
Closed by commit rL328418: [clang-tidy] Enable Python 3 support for
add_new_check.py (authored by jbcoe, committed by ).
Herald added subscribers: llvm-commits, klimek.
Changed prior to commit:
https://reviews.llvm.org/D44217?vs=137646&id=139705#toc
Repository:
rL LLVM
https://reviews.llvm.org/D44217
Files:
clang-tools-extra/trunk/clang-tidy/add_new_check.py
Index: clang-tools-extra/trunk/clang-tidy/add_new_check.py
===
--- clang-tools-extra/trunk/clang-tidy/add_new_check.py
+++ clang-tools-extra/trunk/clang-tidy/add_new_check.py
@@ -9,12 +9,13 @@
#
#======#
+from __future__ import print_function
+
import argparse
import os
import re
import sys
-
# Adapts the module's CMakelist file. Returns 'True' if it could add a new entry
# and 'False' if the entry already existed.
def adapt_cmake(module_path, check_name_camel):
@@ -30,7 +31,7 @@
return False
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
cpp_found = False
file_added = False
for line in lines:
@@ -50,7 +51,7 @@
check_name_dashes = module + '-' + check_name
filename = os.path.join(module_path, check_name_camel) + '.h'
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
header_guard = ('LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_' + module.upper() + '_'
+ check_name_camel.upper() + '_H')
f.write('//===--- ')
@@ -103,7 +104,7 @@
def write_implementation(module_path, module, check_name_camel):
filename = os.path.join(module_path, check_name_camel) + '.cpp'
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write('//===--- ')
f.write(os.path.basename(filename))
f.write(' - clang-tidy')
@@ -152,14 +153,15 @@
# Modifies the module to include the new check.
def adapt_module(module_path, module, check_name, check_name_camel):
- modulecpp = filter(lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
- os.listdir(module_path))[0]
+ modulecpp = list(filter(
+ lambda p: p.lower() == module.lower() + 'tidymodule.cpp',
+ os.listdir(module_path)))[0]
filename = os.path.join(module_path, modulecpp)
with open(filename, 'r') as f:
lines = f.readlines()
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
header_added = False
header_found = False
check_added = False
@@ -199,7 +201,7 @@
lines = f.readlines()
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
note_added = False
header_found = False
@@ -227,7 +229,7 @@
filename = os.path.normpath(os.path.join(module_path, '../../test/clang-tidy',
check_name_dashes + '.' + test_extension))
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write("""// RUN: %%check_clang_tidy %%s %(check_name_dashes)s %%t
// FIXME: Add something that triggers the check here.
@@ -251,8 +253,8 @@
filename = os.path.normpath(os.path.join(docs_dir, 'list.rst'))
with open(filename, 'r') as f:
lines = f.readlines()
- doc_files = filter(lambda s: s.endswith('.rst') and s != 'list.rst',
- os.listdir(docs_dir))
+ doc_files = list(filter(lambda s: s.endswith('.rst') and s != 'list.rst',
+ os.listdir(docs_dir)))
doc_files.sort()
def format_link(doc_file):
@@ -275,7 +277,7 @@
checks = map(format_link, doc_files)
print('Updating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
for line in lines:
f.write(line)
if line.startswith('.. toctree::'):
@@ -289,7 +291,7 @@
filename = os.path.normpath(os.path.join(
module_path, '../../docs/clang-tidy/checks/', check_name_dashes + '.rst'))
print('Creating %s...' % filename)
- with open(filename, 'wb') as f:
+ with open(filename, 'w') as f:
f.write(""".. title:: clang-tidy - %(check_name_dashes)s
%(check_name_dashes)s
@@ -333,16 +335,16 @@
return
if not args.module or not args.check:
-print 'Module and check must be specified.'
+print('Module and check must be specified.')
parser.print_usage()
return
module = args.module
check_name = args.check
if check_name.startswith(module):
-print 'Check name "%s" must not start with the module "%s". Exiting.' % (
-check_name, module)
+print('Check name "%s" must not start with the module "%s". Exiting.' % (
+check_name, module))
return
check_name_camel = ''.join(map(lambda elem: elem.ca
[PATCH] D40854: [clang-tidy] WIP implement cppcoreguidelines check for mixed integer arithmetic
JonasToth updated this revision to Diff 139706.
JonasToth added a comment.
- update to trunk
- [Doc] update to new doc style
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D40854
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.cpp
clang-tidy/cppcoreguidelines/MixedIntArithmeticCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-mixed-int-arithmetic.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp
Index: test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-mixed-int-arithmetic.cpp
@@ -0,0 +1,188 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-mixed-int-arithmetic %t
+
+enum UnsignedEnum : unsigned char {
+ UEnum1,
+ UEnum2
+};
+
+enum SignedEnum : signed char {
+ SEnum1,
+ SEnum2
+};
+
+unsigned char returnUnsignedCharacter() { return 42; }
+unsigned returnUnsignedNumber() { return 42u; }
+long returnBigNumber() { return 42; }
+float unrelatedThing() { return 42.f; }
+SignedEnum returnSignedEnum() { return SEnum1; }
+UnsignedEnum returnUnsignedEnum() { return UEnum1; }
+
+void mixed_binary() {
+ unsigned int UInt1 = 42;
+ signed int SInt1 = 42;
+ UnsignedEnum UE1 = UEnum1;
+ SignedEnum SE1 = SEnum1;
+ float UnrelatedFloat = 42.f;
+
+ // Test traditional integer types.
+ auto R1 = UInt1 + SInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+ int R2 = UInt1 - SInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:12: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:20: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:12: note: unsigned operand
+
+ unsigned int R3 = UInt1 * SInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+ unsigned int R4 = UInt1 / returnBigNumber();
+ // CHECK-MESSAGES: [[@LINE-1]]:21: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:29: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+ char R5 = returnUnsignedCharacter() + SInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:41: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+ auto R6 = SInt1 - 10u;
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:21: note: unsigned operand
+
+ auto R7 = UInt1 * 10;
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:21: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+ auto R8 = 10u / returnBigNumber();
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:19: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:13: note: unsigned operand
+
+ auto R9 = 10 + returnUnsignedCharacter();
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:13: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:18: note: unsigned operand
+
+ // Test enum types.
+ char R10 = returnUnsignedEnum() - SInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:14: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:37: note: signed operand
+ // CHECK-MESSAGES: [[@LINE-3]]:14: note: unsigned operand
+
+ unsigned char R11 = returnSignedEnum() * UInt1;
+ // CHECK-MESSAGES: [[@LINE-1]]:23: warning: mixed signed and unsigned arithmetic; prefer signed integers and use unsigned types only for modulo arithmetic
+ // CHECK-MESSAGES: [[@LINE-2]]:23: note: signed operand
[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules
JonasToth updated this revision to Diff 139707.
JonasToth added a comment.
- Merge branch 'master' into check_macros_usage
- [Doc] update to new doc style
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41648
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
clang-tidy/cppcoreguidelines/MacroUsageCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
test/clang-tidy/cppcoreguidelines-macro-usage.cpp
Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -75,6 +75,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator)
cppcoreguidelines-interfaces-global-init
+ cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Core Guidelines are
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -82,6 +82,12 @@
with looping constructs. Every backward jump is rejected. Forward jumps are
only allowed in nested loops.
+- New :doc:`cppcoreguidelines-macro-usage
+ ` check
+
+ Find macro usage that is considered problematic because better language
+ constructs exist for the task.
+
- New :doc:`fuchsia-multiple-inheritance
` check
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,45 @@
+//===--- MacroUsageCheck.
[PATCH] D44852: [CodeGen] Mark fma as const for Android
spatel accepted this revision. spatel added inline comments. Comment at: lib/Sema/SemaDecl.cpp:13108-13110 // We make "fma" on GNU or Windows const because we know it does not set // errno in those environments even though it could set errno based on the // C standard. Please generalize this comment so it doesn't go stale: "We make fma on some platforms const because..." Repository: rC Clang https://reviews.llvm.org/D44852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.
lebedev.ri updated this revision to Diff 139711.
lebedev.ri marked 15 inline comments as done.
lebedev.ri added a comment.
- Rebased ontop of master
- Fixed links in documentation
- Improved test coverage
- No longer count variables declared in nested `class`es.
- No longer count variables declared in nested lambdas. FIXME: this check()` is
not being called for lambdas at all. But out of the scope of this patch.
- No longer count variables declared in macro expansion. Please see FIXME, i
think this is too broad.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44602
Files:
clang-tidy/readability/FunctionSizeCheck.cpp
clang-tidy/readability/FunctionSizeCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/readability-function-size.rst
test/clang-tidy/readability-function-size-variables-c++17.cpp
test/clang-tidy/readability-function-size.cpp
Index: test/clang-tidy/readability-function-size.cpp
===
--- test/clang-tidy/readability-function-size.cpp
+++ test/clang-tidy/readability-function-size.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-function-size %t -- -config='{CheckOptions: [{key: readability-function-size.LineThreshold, value: 0}, {key: readability-function-size.StatementThreshold, value: 0}, {key: readability-function-size.BranchThreshold, value: 0}, {key: readability-function-size.ParameterThreshold, value: 5}, {key: readability-function-size.NestingThreshold, value: 2}]}' -- -std=c++11
+// RUN: %check_clang_tidy %s readability-function-size %t -- -config='{CheckOptions: [{key: readability-function-size.LineThreshold, value: 0}, {key: readability-function-size.StatementThreshold, value: 0}, {key: readability-function-size.BranchThreshold, value: 0}, {key: readability-function-size.ParameterThreshold, value: 5}, {key: readability-function-size.NestingThreshold, value: 2}, {key: readability-function-size.VariableThreshold, value: 1}]}' -- -std=c++11
// Bad formatting is intentional, don't run clang-format over the whole file!
@@ -64,7 +64,7 @@
void baz0() { // 1
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'baz0' exceeds recommended size/complexity
- // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 27 lines including whitespace and comments (threshold 0)
+ // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 28 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-3]]:6: note: 9 statements (threshold 0)
int a;
{ // 2
@@ -87,14 +87,15 @@
}
}
macro()
-// CHECK-MESSAGES: :[[@LINE-1]]:3: note: nesting level 3 starts here (threshold 2)
-// CHECK-MESSAGES: :[[@LINE-28]]:25: note: expanded from macro 'macro'
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: note: nesting level 3 starts here (threshold 2)
+ // CHECK-MESSAGES: :[[@LINE-28]]:25: note: expanded from macro 'macro'
+ // CHECK-MESSAGES: :[[@LINE-27]]:6: note: 6 variables (threshold 1)
}
// check that nested if's are not reported. this was broken initially
void nesting_if() { // 1
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'nesting_if' exceeds recommended size/complexity
- // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 22 lines including whitespace and comments (threshold 0)
+ // CHECK-MESSAGES: :[[@LINE-2]]:6: note: 23 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-3]]:6: note: 18 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 6 branches (threshold 0)
if (true) { // 2
@@ -114,12 +115,13 @@
} else if (true) { // 2
int j;
}
+ // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 6 variables (threshold 1)
}
// however this should warn
void bad_if_nesting() { // 1
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'bad_if_nesting' exceeds recommended size/complexity
-// CHECK-MESSAGES: :[[@LINE-2]]:6: note: 22 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-2]]:6: note: 23 lines including whitespace and comments (threshold 0)
// CHECK-MESSAGES: :[[@LINE-3]]:6: note: 12 statements (threshold 0)
// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 4 branches (threshold 0)
if (true) {// 2
@@ -139,4 +141,153 @@
}
}
}
+ // CHECK-MESSAGES: :[[@LINE-22]]:6: note: 4 variables (threshold 1)
}
+
+void variables_0() {
+ int i;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_0' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+void variables_1(int i) {
+ int j;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:6: warning: function 'variables_1' exceeds recommended size/complexity thresholds [readability-function-size]
+// CHECK-MESSAGES: :[[@LINE-4]]:6: note: 2 lines including whitespace and comments (threshold 0)
+// CHECK-MESSAGES: :[[@LINE-5]]:6: note: 1 statements (threshold 0)
+vo
[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.
lebedev.ri added inline comments.
Comment at: test/clang-tidy/readability-function-size.cpp:207-212
+void variables_8() {
+ int a, b;
+ struct A {
+A(int c, int d);
+ };
+}
aaron.ballman wrote:
> lebedev.ri wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > lebedev.ri wrote:
> > > > > aaron.ballman wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > I think the current behavior here is correct and the
> > > > > > > > > > > > previous behavior was incorrect. However, it brings up
> > > > > > > > > > > > an interesting question about what to do here:
> > > > > > > > > > > > ```
> > > > > > > > > > > > void f() {
> > > > > > > > > > > > struct S {
> > > > > > > > > > > > void bar() {
> > > > > > > > > > > > int a, b;
> > > > > > > > > > > > }
> > > > > > > > > > > > };
> > > > > > > > > > > > }
> > > > > > > > > > > > ```
> > > > > > > > > > > > Does `f()` contain zero variables or two? I would
> > > > > > > > > > > > contend that it has no variables because S::bar() is a
> > > > > > > > > > > > different scope than f(). But I can see a case being
> > > > > > > > > > > > made about the complexity of f() being increased by the
> > > > > > > > > > > > presence of the local class definition. Perhaps this is
> > > > > > > > > > > > a different facet of the test about number of types?
> > > > > > > > > > > As previously briefly discussed in IRC, i **strongly**
> > > > > > > > > > > believe that the current behavior is correct, and
> > > > > > > > > > > `readability-function-size`
> > > > > > > > > > > should analyze/diagnose the function as a whole,
> > > > > > > > > > > including all sub-classes/sub-functions.
> > > > > > > > > > Do you know of any coding standards related to this check
> > > > > > > > > > that weigh in on this?
> > > > > > > > > >
> > > > > > > > > > What do you think about this:
> > > > > > > > > > ```
> > > > > > > > > > #define SWAP(x, y) ({__typeof__(x) temp = x; x = y; y = x;})
> > > > > > > > > >
> > > > > > > > > > void f() {
> > > > > > > > > > int a = 10, b = 12;
> > > > > > > > > > SWAP(a, b);
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > > Does f() have two variables or three? Should presence of
> > > > > > > > > > the `SWAP` macro cause this code to be more complex due to
> > > > > > > > > > having too many variables?
> > > > > > > > > Datapoint: the doc
> > > > > > > > > (`docs/clang-tidy/checks/readability-function-size.rst`)
> > > > > > > > > actually already states that macros *are* counted.
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > .. option:: StatementThreshold
> > > > > > > > >
> > > > > > > > >Flag functions exceeding this number of statements. This
> > > > > > > > > may differ
> > > > > > > > >significantly from the number of lines for macro-heavy
> > > > > > > > > code. The default is
> > > > > > > > >`800`.
> > > > > > > > > ```
> > > > > > > > > ```
> > > > > > > > > .. option:: NestingThreshold
> > > > > > > > >
> > > > > > > > > Flag compound statements which create next nesting level
> > > > > > > > > after
> > > > > > > > > `NestingThreshold`. This may differ significantly from
> > > > > > > > > the expected value
> > > > > > > > > for macro-heavy code. The default is `-1` (ignore the
> > > > > > > > > nesting level).
> > > > > > > > > ```
> > > > > > > > My concerns relate to what's considered a "variable declared in
> > > > > > > > the body" (per the documentation) in relation to function
> > > > > > > > complexity. To me, if the variable is not accessible lexically
> > > > > > > > within the body of the function, it's not adding to the
> > > > > > > > function's complexity *for local variables*. It may certainly
> > > > > > > > be adding other complexity, of course.
> > > > > > > >
> > > > > > > > I would have a very hard time explaining to a user that
> > > > > > > > variables they cannot see or change (assuming the macro is in a
> > > > > > > > header file out of their control) contribute to their
> > > > > > > > function's complexity. Similarly, I would have difficulty
> > > > > > > > explaining that variables in an locally declared class member
> > > > > > > > function contribute to the number of variables in the outer
> > > > > > > > function body, but the class data members somehow do not.
> > > > > > > >
> > > > > > > > (per the documentation)
> > > > > > >
> > > > > > > Please note that the word `complexity` is not used in the
> > > > > > > **documentation**, only `size` is.
> > > > > > >
> > > > > > > There also is the other side of the coin:
> > > > > > >
> > > > > > > ```
> > > > > > > #define simple_macro_please_ignore \
> > > > > > > the; \
> > > > > > > actual; \
> > > > > > >
[PATCH] D44602: [clang-tidy] readability-function-size: add VariableThreshold param.
JonasToth added inline comments.
Comment at: test/clang-tidy/readability-function-size.cpp:207-212
+void variables_8() {
+ int a, b;
+ struct A {
+A(int c, int d);
+ };
+}
lebedev.ri wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > JonasToth wrote:
> > > > > lebedev.ri wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > I think the current behavior here is correct and the
> > > > > > > > > > > > > previous behavior was incorrect. However, it brings
> > > > > > > > > > > > > up an interesting question about what to do here:
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > void f() {
> > > > > > > > > > > > > struct S {
> > > > > > > > > > > > > void bar() {
> > > > > > > > > > > > > int a, b;
> > > > > > > > > > > > > }
> > > > > > > > > > > > > };
> > > > > > > > > > > > > }
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > Does `f()` contain zero variables or two? I would
> > > > > > > > > > > > > contend that it has no variables because S::bar() is
> > > > > > > > > > > > > a different scope than f(). But I can see a case
> > > > > > > > > > > > > being made about the complexity of f() being
> > > > > > > > > > > > > increased by the presence of the local class
> > > > > > > > > > > > > definition. Perhaps this is a different facet of the
> > > > > > > > > > > > > test about number of types?
> > > > > > > > > > > > As previously briefly discussed in IRC, i **strongly**
> > > > > > > > > > > > believe that the current behavior is correct, and
> > > > > > > > > > > > `readability-function-size`
> > > > > > > > > > > > should analyze/diagnose the function as a whole,
> > > > > > > > > > > > including all sub-classes/sub-functions.
> > > > > > > > > > > Do you know of any coding standards related to this check
> > > > > > > > > > > that weigh in on this?
> > > > > > > > > > >
> > > > > > > > > > > What do you think about this:
> > > > > > > > > > > ```
> > > > > > > > > > > #define SWAP(x, y) ({__typeof__(x) temp = x; x = y; y =
> > > > > > > > > > > x;})
> > > > > > > > > > >
> > > > > > > > > > > void f() {
> > > > > > > > > > > int a = 10, b = 12;
> > > > > > > > > > > SWAP(a, b);
> > > > > > > > > > > }
> > > > > > > > > > > ```
> > > > > > > > > > > Does f() have two variables or three? Should presence of
> > > > > > > > > > > the `SWAP` macro cause this code to be more complex due
> > > > > > > > > > > to having too many variables?
> > > > > > > > > > Datapoint: the doc
> > > > > > > > > > (`docs/clang-tidy/checks/readability-function-size.rst`)
> > > > > > > > > > actually already states that macros *are* counted.
> > > > > > > > > >
> > > > > > > > > > ```
> > > > > > > > > > .. option:: StatementThreshold
> > > > > > > > > >
> > > > > > > > > >Flag functions exceeding this number of statements. This
> > > > > > > > > > may differ
> > > > > > > > > >significantly from the number of lines for macro-heavy
> > > > > > > > > > code. The default is
> > > > > > > > > >`800`.
> > > > > > > > > > ```
> > > > > > > > > > ```
> > > > > > > > > > .. option:: NestingThreshold
> > > > > > > > > >
> > > > > > > > > > Flag compound statements which create next nesting
> > > > > > > > > > level after
> > > > > > > > > > `NestingThreshold`. This may differ significantly from
> > > > > > > > > > the expected value
> > > > > > > > > > for macro-heavy code. The default is `-1` (ignore the
> > > > > > > > > > nesting level).
> > > > > > > > > > ```
> > > > > > > > > My concerns relate to what's considered a "variable declared
> > > > > > > > > in the body" (per the documentation) in relation to function
> > > > > > > > > complexity. To me, if the variable is not accessible
> > > > > > > > > lexically within the body of the function, it's not adding to
> > > > > > > > > the function's complexity *for local variables*. It may
> > > > > > > > > certainly be adding other complexity, of course.
> > > > > > > > >
> > > > > > > > > I would have a very hard time explaining to a user that
> > > > > > > > > variables they cannot see or change (assuming the macro is in
> > > > > > > > > a header file out of their control) contribute to their
> > > > > > > > > function's complexity. Similarly, I would have difficulty
> > > > > > > > > explaining that variables in an locally declared class member
> > > > > > > > > function contribute to the number of variables in the outer
> > > > > > > > > function body, but the class data members somehow do not.
> > > > > > > > >
> > > > > > > > > (per the documentation)
> > > > > > > >
> > > > > > > > Please note that the word `complexity` is not used in the
> > > > > > > > **d
[PATCH] D42490: [cmake] Set cmake policy CMP0068 to suppress warnings on OSX
hintonda abandoned this revision. hintonda added a comment. Not needed for llvm+clang builds. Repository: rC Clang https://reviews.llvm.org/D42490 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43341: [clang-doc] Implement reducer portion of the frontend framework
lebedev.ri added a comment.
It's good to finally have the initial block firmly landed, congratulations.
Trying to review this...
Some initial thoughts.
Comment at: clang-doc/BitcodeReader.cpp:27
+ assert(Record[0] == 20);
+ for (int I = 0, E = Record[0]; I < E; ++I)
+Field[I] = Record[I + 1];
Ok, i don't understand what is going on here.
Where is this `E` defined?
This looks like `[0]` is the number of elements to read (always 20, sha1 blob?),
and it copies Record[1]..Record[20] to Field[0]..Field[19] ?
I think this can/should be rewritten clearer..
Comment at: clang-doc/BitcodeReader.cpp:19
+
+void ClangDocBitcodeReader::storeData(llvm::SmallString<4> &Field,
+ llvm::StringRef Blob) {
juliehockett wrote:
> lebedev.ri wrote:
> > I think all these `SmallString` can be one `llvm::SmallVectorImpl`?
> No, since there's not an implicit converter from
> `llvm::SmallVectorImpl` to `StringRef`. I templatized it on the size
> though, so it's only one function now.
Are you sure?
https://godbolt.org/g/wD1FKD
That isn't pretty, but i think it beats templating in this case..
Comment at: clang-doc/BitcodeReader.h:33
+
+ bool readBitstream(std::unique_ptr &IS);
+
Similarly, i think this should be
```
bool readBitstream(InfoSet &IS);
```
Comment at: clang-doc/BitcodeReader.h:44
+ template
+ bool readBlockToInfo(unsigned ID, std::unique_ptr &IS);
+
As far as i can see this should be
```
template
bool readBlockToInfo(unsigned ID, InfoSet &IS);
```
Comment at: clang-doc/BitcodeWriter.h:142
void emitBlock(const CommentInfo &B);
+ void emitInfoSet(std::unique_ptr &ISet);
And here too, it does not make much sense to call it with empty
`std::unique_ptr<>`, so maybe
```
void emitInfoSet(InfoSet &ISet);
```
?
Comment at: clang-doc/Reducer.cpp:18
+std::unique_ptr mergeInfos(tooling::ToolResults *Results) {
+ std::unique_ptr UniqueInfos = llvm::make_unique();
+ bool Err = false;
I can see that you may `return nullptr;` in case of error here, thus it's
`std::unique_ptr<>`
`InfoSet` internally is just a few `std::vector<>`s, so it should `std::move()`
efficiently.
I'm wondering if `llvm::Optional` would work too?
Comment at: clang-doc/Representation.cpp:19
+ SymbolID USR;
+ std::string HexString = fromHex(StringUSR);
+ std::copy(HexString.begin(), HexString.end(), USR.begin());
I though this was changed, and the non-stringified, original binary version of
sha1 was emitted into bitcode?
Comment at: clang-doc/Representation.cpp:28
+ if (L.Namespace.empty())
+std::move(R.Namespace);
+ std::move(R.Description.begin(), R.Description.end(),
???
Where *to* is it moved?
Comment at: clang-doc/Representation.cpp:40
+
+static void mergeInfo(NamespaceInfo &L, NamespaceInfo &R) {
+ mergeInfoBase(L, R);
All these `mergeInfo()`: the second param, `Info &R` should probably be `Info
&&R`.
(but not `mergeInfoBase()`/`mergeSymbolInfoBase()`)
Comment at: clang-doc/Representation.cpp:98
+else
\
+ mergeInfo(X##s[R.first->second], I);
\
+ }
Seeing how many `std::move()`'ing is happening in those `mergeInfo()`, i think
you want to move `I`, not pass as reference.
Especially since it is already `&&I` in this `InfoSet::insert()` function.
Comment at: clang-doc/Representation.h:30
using SymbolID = std::array;
+SymbolID StringToSymbol(llvm::StringRef StringUSR);
Please add a comment explaining that `SymbolID` is sha1, and not hex-string of
sha1.
Comment at: clang-doc/Representation.h:135
Info() = default;
- Info(Info &&Other) : Description(std::move(Other.Description)) {}
- virtual ~Info() = default;
+ Info(Info &&Other)
+ : USR(Other.USR), Name(Other.Name),
Namespace(std::move(Other.Namespace)),
Why is the move constructor explicitly defined?
Unless i'm missing something, the default one would do exactly the same.
https://godbolt.org/g/XqsRuX
Comment at: clang-doc/tool/ClangDocMain.cpp:130
+llvm::outs() << "Writing intermediate results...\n";
+SmallString<2048> Buffer;
+llvm::BitstreamWriter Stream(Buffer);
That is the same small-size used in `static std::string serialize(T &I)`.
Some statistic is needed, bu i think this one can be bumped somewhat right away.
Comment at: clang-doc/tool/ClangDocMain.cpp:141
+ llvm::errs() << "Unable to create documentation directories.\n";
+ return 1;
+}
This shares some code with `if(DumpMapperResu
[PATCH] D43696: Reduce hash collisions for reference and pointer types
v.g.vassilev abandoned this revision. v.g.vassilev added a comment. Ok, that's great! Sorry for the delay and thanks for landing a similar patch. Btw, we should probably find a more terse way to test the ODRHash, eg. with unittests. Repository: rC Clang https://reviews.llvm.org/D43696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r328245 - Use DoNotOptimize to prevent new/delete elision.
Hi, I am seeing failure on this test too. test\std\utilities\memory\default.allocator\allocator. members\construct.pass.cpp For the same reason : new optimized away. On Thu, Mar 22, 2018 at 5:28 PM, Eric Fiselier via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: ericwf > Date: Thu Mar 22 14:28:09 2018 > New Revision: 328245 > > URL: http://llvm.org/viewvc/llvm-project?rev=328245&view=rev > Log: > Use DoNotOptimize to prevent new/delete elision. > > The new/delete tests, in particular those which test replacement > functions, often fail when the optimizer is enabled because the > calls to new/delete may be optimized away, regardless of their > side-effects. > > This patch converts the tests to use DoNotOptimize in order to prevent > the elision. > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r328245 - Use DoNotOptimize to prevent new/delete elision.
Still? I would of hoped this patch fixed it. Could you provide the test output? /Eric On Sat, Mar 24, 2018 at 4:39 PM, Francois Pichet wrote: > Hi, > > I am seeing failure on this test too. > test\std\utilities\memory\default.allocator\allocator.member > s\construct.pass.cpp > > For the same reason : new optimized away. > > > On Thu, Mar 22, 2018 at 5:28 PM, Eric Fiselier via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: ericwf >> Date: Thu Mar 22 14:28:09 2018 >> New Revision: 328245 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=328245&view=rev >> Log: >> Use DoNotOptimize to prevent new/delete elision. >> >> The new/delete tests, in particular those which test replacement >> functions, often fail when the optimizer is enabled because the >> calls to new/delete may be optimized away, regardless of their >> side-effects. >> >> This patch converts the tests to use DoNotOptimize in order to prevent >> the elision. >> >> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r328245 - Use DoNotOptimize to prevent new/delete elision.
You didn't update allocator.members\construct.pass.cpp in your patch. On Sat, Mar 24, 2018 at 6:41 PM, Eric Fiselier wrote: > Still? > > I would of hoped this patch fixed it. > > Could you provide the test output? > > /Eric > > On Sat, Mar 24, 2018 at 4:39 PM, Francois Pichet > wrote: > >> Hi, >> >> I am seeing failure on this test too. >> test\std\utilities\memory\default.allocator\allocator.member >> s\construct.pass.cpp >> >> For the same reason : new optimized away. >> >> >> On Thu, Mar 22, 2018 at 5:28 PM, Eric Fiselier via cfe-commits < >> cfe-commits@lists.llvm.org> wrote: >> >>> Author: ericwf >>> Date: Thu Mar 22 14:28:09 2018 >>> New Revision: 328245 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=328245&view=rev >>> Log: >>> Use DoNotOptimize to prevent new/delete elision. >>> >>> The new/delete tests, in particular those which test replacement >>> functions, often fail when the optimizer is enabled because the >>> calls to new/delete may be optimized away, regardless of their >>> side-effects. >>> >>> This patch converts the tests to use DoNotOptimize in order to prevent >>> the elision. >>> >>> >>> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[libcxx] r328445 - avoid new/delete ellision in construct.pass.cpp
Author: ericwf Date: Sat Mar 24 20:00:42 2018 New Revision: 328445 URL: http://llvm.org/viewvc/llvm-project?rev=328445&view=rev Log: avoid new/delete ellision in construct.pass.cpp Modified: libcxx/trunk/test/std/utilities/memory/default.allocator/allocator.members/construct.pass.cpp Modified: libcxx/trunk/test/std/utilities/memory/default.allocator/allocator.members/construct.pass.cpp URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/utilities/memory/default.allocator/allocator.members/construct.pass.cpp?rev=328445&r1=328444&r2=328445&view=diff == --- libcxx/trunk/test/std/utilities/memory/default.allocator/allocator.members/construct.pass.cpp (original) +++ libcxx/trunk/test/std/utilities/memory/default.allocator/allocator.members/construct.pass.cpp Sat Mar 24 20:00:42 2018 @@ -63,6 +63,7 @@ int main() globalMemCounter.last_new_size = 0; A* ap = a.allocate(3); +DoNotOptimize(ap); assert(globalMemCounter.checkOutstandingNewEq(1)); assert(globalMemCounter.checkLastNewSizeEq(3 * sizeof(int))); assert(A_constructed == 0); @@ -100,6 +101,7 @@ int main() assert(A_constructed == 0); a.deallocate(ap, 3); +DoNotOptimize(ap); assert(globalMemCounter.checkOutstandingNewEq(0)); assert(A_constructed == 0); } @@ -111,6 +113,7 @@ int main() globalMemCounter.last_new_size = 0; move_only* ap = a.allocate(3); +DoNotOptimize(ap); assert(globalMemCounter.checkOutstandingNewEq(1)); assert(globalMemCounter.checkLastNewSizeEq(3 * sizeof(int))); assert(move_only_constructed == 0); @@ -132,6 +135,7 @@ int main() assert(move_only_constructed == 0); a.deallocate(ap, 3); +DoNotOptimize(ap); assert(globalMemCounter.checkOutstandingNewEq(0)); assert(move_only_constructed == 0); } ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [libcxx] r328245 - Use DoNotOptimize to prevent new/delete elision.
Woops. I misread the name of the test. I just committed a fix. Thanks for the heads up. /Eric On Sat, Mar 24, 2018 at 4:44 PM, Francois Pichet wrote: > You didn't update allocator.members\construct.pass.cpp in your patch. > > > On Sat, Mar 24, 2018 at 6:41 PM, Eric Fiselier wrote: > >> Still? >> >> I would of hoped this patch fixed it. >> >> Could you provide the test output? >> >> /Eric >> >> On Sat, Mar 24, 2018 at 4:39 PM, Francois Pichet >> wrote: >> >>> Hi, >>> >>> I am seeing failure on this test too. >>> test\std\utilities\memory\default.allocator\allocator.member >>> s\construct.pass.cpp >>> >>> For the same reason : new optimized away. >>> >>> >>> On Thu, Mar 22, 2018 at 5:28 PM, Eric Fiselier via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> Author: ericwf Date: Thu Mar 22 14:28:09 2018 New Revision: 328245 URL: http://llvm.org/viewvc/llvm-project?rev=328245&view=rev Log: Use DoNotOptimize to prevent new/delete elision. The new/delete tests, in particular those which test replacement functions, often fail when the optimizer is enabled because the calls to new/delete may be optimized away, regardless of their side-effects. This patch converts the tests to use DoNotOptimize in order to prevent the elision. >>> >> > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
