[PATCH] D49289: [mips64][clang] Provide the signext attribute for i32 return values
atanasyan accepted this revision. atanasyan added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D49289 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1157
+ /// language address space.
+ virtual LangAS getCUDABuiltinAddressSpace(unsigned AS) const {
+return getLangASFromTargetAS(AS);
yaxunl wrote:
> I think this function is not needed.
>
> Although CUDA/HIP uses address spaces in codegen, but it does not use named
> address spaces in sema and AST. Named address space is not in the AST types
> of CUDA/HIP, therefore there is no point of mapping target address space back
> to language address space for CUDA/HIP.
>
> CUDA/HIP should be just like other address-space-agnostic language and always
> use getLangASFromTargetAS(AS).
This is necessary to insert the correct addrspacecast, otherwise the builtins
CUDA test asserts. getLangASFromTargetAS returns the user specified addrspace
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm updated this revision to Diff 157885.
arsenm added a comment.
Remove old run line
https://reviews.llvm.org/D47154
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/BuiltinsAMDGPU.def
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenCUDA/builtins-amdgcn.cu
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/numbered-address-space.cl
test/SemaOpenCL/numbered-address-space.cl
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-amd-amdhsa -verify -pedantic -fsyntax-only %s
+
+void test_numeric_as_to_generic_implicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+}
+
+void test_numeric_as_to_generic_explicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = (generic int*) as3_ptr; // Should maybe be valid?
+}
+
+void test_generic_to_numeric_as_implicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = generic_ptr; // expected-error{{initializing '__attribute__((address_space(3))) int *' with an expression of type '__generic int *' changes address space of pointer}}
+}
+
+void test_generic_to_numeric_as_explicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = (__attribute__((address_space(3))) int *)generic_ptr;
+}
+
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__attribute__((address_space(3))) float*) generic_ptr, src, 0, 0, false); // expected-error {{passing '__attribute__((address_space(3))) float *' to parameter of type '__local float *' changes address space of pointer}}
+}
+
+void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+}
+
Index: test/CodeGenOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/numbered-address-space.cl
@@ -0,0 +1,34 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -target-cpu tonga -S -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Make sure using numbered address spaces doesn't trigger crashes when a
+// builtin has an address space parameter.
+
+// CHECK-LABEL: @test_numbered_as_to_generic(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to i32*
+void test_numbered_as_to_generic(__attribute__((address_space(42))) int *arbitary_numbered_ptr) {
+ generic int* generic_ptr = arbitary_numbered_ptr;
+ *generic_ptr = 4;
+}
+
+// CHECK-LABEL: @test_numbered_as_to_builtin(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to float addrspace(3)*
+void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
+ volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_explicit_cast(
+// CHECK: addrspacecast i32 addrspace(3)* %0 to i32*
+void test_generic_as_to_builtin_parameter_explicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__local float*) generic_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_implicit_cast(
+// CHECK: addrspacecast i32* %2 to float addrspace(3)*
+void test_generic_as_to_builtin_parameter_implicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false);
+}
+
Index: test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- test/CodeGenOpenCL/builtins-amdgcn.cl
+++ test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -1,6 +1,5 @@
// REQUIRES: amdgpu-registered-target
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s
#pragma OPENCL EXTENSION cl_khr_fp64 : enable
@@ -20,19 +19,42
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added a comment.
Thank you for working on this, I think it's getting closer! I'd use a slightly
different approach to handling floating-point values, but if that turns out to
be a clean implementation we may want to think about whether there are
improvements from modelling integers similarly (only using `APInt` instead of
`APFloat`).
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
I would still like to see some data on common floating-point literal values
used in large open source project so that we can see what sensible values
should be in this list.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:63
+ Options.get("IgnoreAllFloatingPointValues", false)) {
+ // process set of ignored integer values
+ const std::vector IgnoredIntegerValuesInput =
Comments should be grammatically correct, including capitalization and
punctuation (elsewhere in this function as well).
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
This is where I would construct an `APFloat` object from the string given. As
for the semantics to be used, I would recommend getting it from
`TargetInfo::getDoubleFormat()` on the belief that we aren't going to care
about precision (explained in the documentation).
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:128-139
+ if (FloatValue.isZero())
+return true;
+ else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEsingle()) {
+const float Value = FloatValue.convertToFloat();
+return std::binary_search(IgnoredFloatingPointValues.begin(),
+ IgnoredFloatingPointValues.end(), Value);
+ } else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble()) {
Here is where I would compare `FloatValue` against everything in the
`IgnoredFloatingPointValues` array using `APFloat::compare()`. However, you
need the floats to have the same semantics for that to work, so you'd have to
convert `FloatValue` using `APFloat::convert()` to whatever the target
floating-point format is.
Comment at: clang-tidy/readability/MagicNumbersCheck.h:85-88
+ llvm::SmallVector
+ IgnoredFloatingPointValues;
+ llvm::SmallVector
+ IgnoredDoublePointValues;
The model I was thinking of would have this be: `llvm::SmallVector IgnoredFloatingPointValues;` so that you
don't need to distinguish between floats and doubles (which also nicely
supports long doubles, __float128, and _Float16).
Comment at: docs/clang-tidy/checks/readability-magic-numbers.rst:63-67
+be configured using the :option:`IgnoredFloatingPointValues` option. For each
+value in that set, bot the single-precision form and double-precision
+form are accepted (for example, if 3.14 is in the set, neither 3.14f nor 3.14
+will produce a warning). Scientific notation is supported for both source code
+input and option.
This isn't far off the mark, but I'd go with something like:
```
For each value in that set, the given string value is converted to a
floating-point value representation used by the target architecture . If a
floating-point literal value compares equal to one of the converted values,
then that literal is not diagnosed by this check. Because floating-point
equality is used to determine whether to diagnose or not, the user needs to be
aware of the details of floating-point representations for any values that
cannot be precisely represented for their target architecture.
```
The bit is where we can stick details we haven't worked out yet, like
what rounding mode is used to perform the conversion and whether lossy
conversions are allowed or ignored.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- updated this revision to Diff 157886.
0x8000- added a comment.
Indicate that `0` and `0.0` are accepted unconditionally (because it makes
sense in the source code, and speeds-up many checks as 0s are very common and
we don't want to spend log2(n) to find them at the beginning of the vector).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
Files:
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MagicNumbersCheck.cpp
clang-tidy/readability/MagicNumbersCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-magic-numbers.rst
test/clang-tidy/readability-magic-numbers.cpp
Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,194 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}]}' \
+// RUN: --
+
+template
+struct ValueBucket {
+ T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+ return param * param;
+}
+
+void BuggyFunction() {
+ int BadLocalInt = 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ (void)IntSquarer(7);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int LocalArray[8];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ for (int ii = 0; ii < 8; ++ii)
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 8 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ }
+
+ ValueBucket Bucket;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 4 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+ TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int getValue() const;
+
+private:
+ int oneMember = 9;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int anotherMember;
+
+ int yetAnotherMember;
+
+ const int oneConstant = 2;
+
+ const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+ if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+ return -3; // FILENOTFOUND
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+ const int GoodLocalIntConstant = 43;
+
+ (void)IntSquarer(GoodLocalIntConstant);
+
+ int LocalArray[INT_MACRO];
+
+ ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100};
+
+float GrandfatheredFloatValues[] = { 3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4 };
+
+/*
+ * no warnings for enums
+ */
+enum Sm
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- marked 4 inline comments as done.
0x8000- added a comment.
See inline comments. Basically we need two arrays because APFloats of different
semantics don't compare well, and even if we coerce them, they sometimes are
not equal.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
aaron.ballman wrote:
> I would still like to see some data on common floating-point literal values
> used in large open source project so that we can see what sensible values
> should be in this list.
What value would that bring? The ideal target is that there are no magic values
- no guideline that I have seen makes exception for 3.141 or 9.81. Each project
is special based on how they evolved, and they need to decide for themselves
what is worth cleaning vs what can be swept under the rug for now. Why would we
lend authority to any particular floating point value?
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
aaron.ballman wrote:
> This is where I would construct an `APFloat` object from the string given. As
> for the semantics to be used, I would recommend getting it from
> `TargetInfo::getDoubleFormat()` on the belief that we aren't going to care
> about precision (explained in the documentation).
Here is the problem I tried to explain last night but perhaps I wasn't clear
enough.
When we parse the input list from strings, we have to commit to one floating
point value "semantic" - in our case single or double precision.
When we encounter the value in the source code and it is captured by a matcher,
it comes as either one of those values.
Floats with different semantics can't be directly compared - so we have to
maintain two distinct arrays.
If we do that, rather than store APFloats and sort/compare them with awkward
lambdas, we might as well just use the native float/double and be done with it
more cleanly.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:128-139
+ if (FloatValue.isZero())
+return true;
+ else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEsingle()) {
+const float Value = FloatValue.convertToFloat();
+return std::binary_search(IgnoredFloatingPointValues.begin(),
+ IgnoredFloatingPointValues.end(), Value);
+ } else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble()) {
aaron.ballman wrote:
> Here is where I would compare `FloatValue` against everything in the
> `IgnoredFloatingPointValues` array using `APFloat::compare()`. However, you
> need the floats to have the same semantics for that to work, so you'd have to
> convert `FloatValue` using `APFloat::convert()` to whatever the target
> floating-point format is.
If you do that, 3.14f will not be equal to 3.14 . You need two arrays.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
0x8000- wrote:
> aaron.ballman wrote:
> > I would still like to see some data on common floating-point literal values
> > used in large open source project so that we can see what sensible values
> > should be in this list.
> What value would that bring? The ideal target is that there are no magic
> values - no guideline that I have seen makes exception for 3.141 or 9.81.
> Each project is special based on how they evolved, and they need to decide
> for themselves what is worth cleaning vs what can be swept under the rug for
> now. Why would we lend authority to any particular floating point value?
Because that's too high of a high false positive rate for an acceptable
clang-tidy check. As mentioned before, there are literally hundreds of
unnameable floating-point literals in LLVM alone where the value is 1.0 or 2.0.
Having statistical data to pick sensible defaults for this list is valuable in
that it lowers the false positive rate. If the user dislikes the default list
for some reason (because for their project, maybe 2.0 is a supremely nameable
literal value), they can pick a different set of defaults.
Right now, I'm operating off an assumption that most floating-point literals
that should not be named are going to be whole numbers that are precisely
represented in all floating-point semantic models. This data will tell us if
that assumption is wrong, and if the assumption is wrong, we might want to go
with separate lists like you've done.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> aaron.ballman wrote:
> > This is where I would construct an `APFloat` object from the string given.
> > As for the semantics to be used, I would recommend getting it from
> > `TargetInfo::getDoubleFormat()` on the belief that we aren't going to care
> > about precision (explained in the documentation).
> Here is the problem I tried to explain last night but perhaps I wasn't clear
> enough.
>
> When we parse the input list from strings, we have to commit to one floating
> point value "semantic" - in our case single or double precision.
>
> When we encounter the value in the source code and it is captured by a
> matcher, it comes as either one of those values.
>
> Floats with different semantics can't be directly compared - so we have to
> maintain two distinct arrays.
>
> If we do that, rather than store APFloats and sort/compare them with awkward
> lambdas, we might as well just use the native float/double and be done with
> it more cleanly.
>When we encounter the value in the source code and it is captured by a
>matcher, it comes as either one of those values.
It may also come in as long double or __float128, for instance, because there
are type suffixes for that.
> Floats with different semantics can't be directly compared - so we have to
> maintain two distinct arrays.
Yes, floats with different semantics cannot be directly compared. That's why I
said below that we should coerce the literal values.
> If we do that, rather than store APFloats and sort/compare them with awkward
> lambdas, we might as well just use the native float/double and be done with
> it more cleanly.
There are too many different floating-point semantics for this to be viable,
hence why coercion is a reasonable behavior.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:128-139
+ if (FloatValue.isZero())
+return true;
+ else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEsingle()) {
+const float Value = FloatValue.convertToFloat();
+return std::binary_search(IgnoredFloatingPointValues.begin(),
+ IgnoredFloatingPointValues.end(), Value);
+ } else if (&FloatValue.getSemantics() == &llvm::APFloat::IEEEdouble()) {
0x8000- wrote:
> aaron.ballman wrote:
> > Here is where I would compare `FloatValue` against everything in the
> > `IgnoredFloatingPointValues` array using `APFloat::compare()`. However, you
> > need the floats to have the same semantics for that to work, so you'd have
> > to convert `FloatValue` using `APFloat::convert()` to whatever the target
> > floating-point format is.
> If you do that, 3.14f will not be equal to 3.14 . You need two arrays.
The point to a named c
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- marked 2 inline comments as done.
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > This is where I would construct an `APFloat` object from the string
> > > given. As for the semantics to be used, I would recommend getting it from
> > > `TargetInfo::getDoubleFormat()` on the belief that we aren't going to
> > > care about precision (explained in the documentation).
> > Here is the problem I tried to explain last night but perhaps I wasn't
> > clear enough.
> >
> > When we parse the input list from strings, we have to commit to one
> > floating point value "semantic" - in our case single or double precision.
> >
> > When we encounter the value in the source code and it is captured by a
> > matcher, it comes as either one of those values.
> >
> > Floats with different semantics can't be directly compared - so we have to
> > maintain two distinct arrays.
> >
> > If we do that, rather than store APFloats and sort/compare them with
> > awkward lambdas, we might as well just use the native float/double and be
> > done with it more cleanly.
> >When we encounter the value in the source code and it is captured by a
> >matcher, it comes as either one of those values.
>
> It may also come in as long double or __float128, for instance, because there
> are type suffixes for that.
>
> > Floats with different semantics can't be directly compared - so we have to
> > maintain two distinct arrays.
>
> Yes, floats with different semantics cannot be directly compared. That's why
> I said below that we should coerce the literal values.
>
> > If we do that, rather than store APFloats and sort/compare them with
> > awkward lambdas, we might as well just use the native float/double and be
> > done with it more cleanly.
>
> There are too many different floating-point semantics for this to be viable,
> hence why coercion is a reasonable behavior.
Let me see if I understood it - your proposal is: store only doubles, and when
a floating-point literal is encountered in code, do not use the FloatingLiteral
instance, but parse it again into a double and compare exactly. If the
comparison matches - ignore it.
In that case what is the value of storing APFloats with double semantics in the
IgnoredValues array, instead of doubles?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > This is where I would construct an `APFloat` object from the string
> > > > given. As for the semantics to be used, I would recommend getting it
> > > > from `TargetInfo::getDoubleFormat()` on the belief that we aren't going
> > > > to care about precision (explained in the documentation).
> > > Here is the problem I tried to explain last night but perhaps I wasn't
> > > clear enough.
> > >
> > > When we parse the input list from strings, we have to commit to one
> > > floating point value "semantic" - in our case single or double precision.
> > >
> > > When we encounter the value in the source code and it is captured by a
> > > matcher, it comes as either one of those values.
> > >
> > > Floats with different semantics can't be directly compared - so we have
> > > to maintain two distinct arrays.
> > >
> > > If we do that, rather than store APFloats and sort/compare them with
> > > awkward lambdas, we might as well just use the native float/double and be
> > > done with it more cleanly.
> > >When we encounter the value in the source code and it is captured by a
> > >matcher, it comes as either one of those values.
> >
> > It may also come in as long double or __float128, for instance, because
> > there are type suffixes for that.
> >
> > > Floats with different semantics can't be directly compared - so we have
> > > to maintain two distinct arrays.
> >
> > Yes, floats with different semantics cannot be directly compared. That's
> > why I said below that we should coerce the literal values.
> >
> > > If we do that, rather than store APFloats and sort/compare them with
> > > awkward lambdas, we might as well just use the native float/double and be
> > > done with it more cleanly.
> >
> > There are too many different floating-point semantics for this to be
> > viable, hence why coercion is a reasonable behavior.
> Let me see if I understood it - your proposal is: store only doubles, and
> when a floating-point literal is encountered in code, do not use the
> FloatingLiteral instance, but parse it again into a double and compare
> exactly. If the comparison matches - ignore it.
>
> In that case what is the value of storing APFloats with double semantics in
> the IgnoredValues array, instead of doubles?
> Let me see if I understood it - your proposal is: store only doubles, and
> when a floating-point literal is encountered in code, do not use the
> FloatingLiteral instance, but parse it again into a double and compare
> exactly. If the comparison matches - ignore it.
My proposal is to use `APFloat` as the storage and comparison medium. Read in
strings from the configuration and convert them to an `APFloat` that has double
semantics. Read in literals and call `FloatLiteral::getValue()` to get the
`APFloat` from it, convert it to one that has double semantics as needed, then
perform the comparison between those two `APFloat` objects.
> In that case what is the value of storing APFloats with double semantics in
> the IgnoredValues array, instead of doubles?
Mostly that it allows us to modify or extend the check for more complicated
semantics in the future. Also, it's good practice to use something with
consistent semantic behavior across hosts and targets (comparisons between
numbers that cannot be precisely represented will at least be consistently
compared across hosts when compiling for the same target).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57
+const char DefaultIgnoredIntegerValues[] = "0;1;";
+const char DefaultIgnoredFloatingPointValues[] = "0.0;";
+
aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > I would still like to see some data on common floating-point literal
> > > values used in large open source project so that we can see what sensible
> > > values should be in this list.
> > What value would that bring? The ideal target is that there are no magic
> > values - no guideline that I have seen makes exception for 3.141 or 9.81.
> > Each project is special based on how they evolved, and they need to decide
> > for themselves what is worth cleaning vs what can be swept under the rug
> > for now. Why would we lend authority to any particular floating point value?
> Because that's too high of a high false positive rate for an acceptable
> clang-tidy check. As mentioned before, there are literally hundreds of
> unnameable floating-point literals in LLVM alone where the value is 1.0 or
> 2.0. Having statistical data to pick sensible defaults for this list is
> valuable in that it lowers the false positive rate. If the user dislikes the
> default list for some reason (because for their project, maybe 2.0 is a
> supremely nameable literal value), they can pick a different set of defaults.
>
> Right now, I'm operating off an assumption that most floating-point literals
> that should not be named are going to be whole numbers that are precisely
> represented in all floating-point semantic models. This data will tell us if
> that assumption is wrong, and if the assumption is wrong, we might want to go
> with separate lists like you've done.
Here are the results with the check as-is, run on the llvm code base as of last
night:
top-40
```
10435 2
5543 4
4629 8
3271 3
2702 16
1876 32
1324 64
1309 10
1207 5
1116 128
966 6
733 7
575 256
421 20
406 12
339 9
331 1024
311 100
281 42
253 11
226 15
189 40
172 24
169 0xff
168 13
168 0x80
166 512
137 1.0
133 14
132 31
129 0xDEADBEEF
120 18
120 17
120 1000
115 4096
100 30
94 60
94 0x1234
89 0x20
86 0xFF
```
1.0 is in position 28 with 137 occurrences
2.0 is in position 93 with 27 occurrences
100.0 is in position 96 with 26 occurences
1.0f is in position 182 with 11 occurences
we also have 2.0e0 four times :)
This data suggests that there would be value in a IgnorePowerOf2IntegerLiterals
option.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
aaron.ballman wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > This is where I would construct an `APFloat` object from the string
> > > > > given. As for the semantics to be used, I would recommend getting it
> > > > > from `TargetInfo::getDoubleFormat()` on the belief that we aren't
> > > > > going to care about precision (explained in the documentation).
> > > > Here is the problem I tried to explain last night but perhaps I wasn't
> > > > clear enough.
> > > >
> > > > When we parse the input list from strings, we have to commit to one
> > > > floating point value "semantic" - in our case single or double
> > > > precision.
> > > >
> > > > When we encounter the value in the source code and it is captured by a
> > > > matcher, it comes as either one of those values.
> > > >
> > > > Floats with different semantics can't be directly compared - so we have
> > > > to maintain two distinct arrays.
> > > >
> > > > If we do that, rather than store APFloats and sort/compare them with
> > > > awkward lambdas, we might as well just use the native float/double and
> > > > be done with it more cleanly.
> > > >When we encounter the value in the source code and it is captured by a
> > > >matcher, it comes as either one of those values.
> > >
> > > It may also come in as long double or __float128, for instance, because
> > > there are type suffixes for that.
> > >
> > > > Floats with different semantics can't be directly compared - so we have
> > > > to maintain two distinct arrays.
> > >
> > > Yes, floats with different semantics cannot be directly compared. That's
> > > why I said below that we should coerce the literal values.
> > >
> > > > If we do that, rather than store APFloats and sort/compare them
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added inline comments. Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57 +const char DefaultIgnoredIntegerValues[] = "0;1;"; +const char DefaultIgnoredFloatingPointValues[] = "0.0;"; + 0x8000- wrote: > aaron.ballman wrote: > > 0x8000- wrote: > > > aaron.ballman wrote: > > > > I would still like to see some data on common floating-point literal > > > > values used in large open source project so that we can see what > > > > sensible values should be in this list. > > > What value would that bring? The ideal target is that there are no magic > > > values - no guideline that I have seen makes exception for 3.141 or 9.81. > > > Each project is special based on how they evolved, and they need to > > > decide for themselves what is worth cleaning vs what can be swept under > > > the rug for now. Why would we lend authority to any particular floating > > > point value? > > Because that's too high of a high false positive rate for an acceptable > > clang-tidy check. As mentioned before, there are literally hundreds of > > unnameable floating-point literals in LLVM alone where the value is 1.0 or > > 2.0. Having statistical data to pick sensible defaults for this list is > > valuable in that it lowers the false positive rate. If the user dislikes > > the default list for some reason (because for their project, maybe 2.0 is a > > supremely nameable literal value), they can pick a different set of > > defaults. > > > > Right now, I'm operating off an assumption that most floating-point > > literals that should not be named are going to be whole numbers that are > > precisely represented in all floating-point semantic models. This data will > > tell us if that assumption is wrong, and if the assumption is wrong, we > > might want to go with separate lists like you've done. > Here are the results with the check as-is, run on the llvm code base as of > last night: > > top-40 > ``` > 10435 2 >5543 4 >4629 8 >3271 3 >2702 16 >1876 32 >1324 64 >1309 10 >1207 5 >1116 128 > 966 6 > 733 7 > 575 256 > 421 20 > 406 12 > 339 9 > 331 1024 > 311 100 > 281 42 > 253 11 > 226 15 > 189 40 > 172 24 > 169 0xff > 168 13 > 168 0x80 > 166 512 > 137 1.0 > 133 14 > 132 31 > 129 0xDEADBEEF > 120 18 > 120 17 > 120 1000 > 115 4096 > 100 30 > 94 60 > 94 0x1234 > 89 0x20 > 86 0xFF > ``` > > 1.0 is in position 28 with 137 occurrences > 2.0 is in position 93 with 27 occurrences > 100.0 is in position 96 with 26 occurences > 1.0f is in position 182 with 11 occurences > > we also have 2.0e0 four times :) > > This data suggests that there would be value in a > IgnorePowerOf2IntegerLiterals option. Any chance you can hack the check run on LLVM where it doesn't report any integer values (I'm curious about the top ten or so for floating-point values)? Additionally, it'd be great to get similar numbers from some other projects, like https://github.com/qt just to see how it compares (I've used `bear` to get compile_commands.json file out of Qt before, so I would guess that would work here). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments. Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:57 +const char DefaultIgnoredIntegerValues[] = "0;1;"; +const char DefaultIgnoredFloatingPointValues[] = "0.0;"; + aaron.ballman wrote: > 0x8000- wrote: > > aaron.ballman wrote: > > > 0x8000- wrote: > > > > aaron.ballman wrote: > > > > > I would still like to see some data on common floating-point literal > > > > > values used in large open source project so that we can see what > > > > > sensible values should be in this list. > > > > What value would that bring? The ideal target is that there are no > > > > magic values - no guideline that I have seen makes exception for 3.141 > > > > or 9.81. Each project is special based on how they evolved, and they > > > > need to decide for themselves what is worth cleaning vs what can be > > > > swept under the rug for now. Why would we lend authority to any > > > > particular floating point value? > > > Because that's too high of a high false positive rate for an acceptable > > > clang-tidy check. As mentioned before, there are literally hundreds of > > > unnameable floating-point literals in LLVM alone where the value is 1.0 > > > or 2.0. Having statistical data to pick sensible defaults for this list > > > is valuable in that it lowers the false positive rate. If the user > > > dislikes the default list for some reason (because for their project, > > > maybe 2.0 is a supremely nameable literal value), they can pick a > > > different set of defaults. > > > > > > Right now, I'm operating off an assumption that most floating-point > > > literals that should not be named are going to be whole numbers that are > > > precisely represented in all floating-point semantic models. This data > > > will tell us if that assumption is wrong, and if the assumption is wrong, > > > we might want to go with separate lists like you've done. > > Here are the results with the check as-is, run on the llvm code base as of > > last night: > > > > top-40 > > ``` > > 10435 2 > >5543 4 > >4629 8 > >3271 3 > >2702 16 > >1876 32 > >1324 64 > >1309 10 > >1207 5 > >1116 128 > > 966 6 > > 733 7 > > 575 256 > > 421 20 > > 406 12 > > 339 9 > > 331 1024 > > 311 100 > > 281 42 > > 253 11 > > 226 15 > > 189 40 > > 172 24 > > 169 0xff > > 168 13 > > 168 0x80 > > 166 512 > > 137 1.0 > > 133 14 > > 132 31 > > 129 0xDEADBEEF > > 120 18 > > 120 17 > > 120 1000 > > 115 4096 > > 100 30 > > 94 60 > > 94 0x1234 > > 89 0x20 > > 86 0xFF > > ``` > > > > 1.0 is in position 28 with 137 occurrences > > 2.0 is in position 93 with 27 occurrences > > 100.0 is in position 96 with 26 occurences > > 1.0f is in position 182 with 11 occurences > > > > we also have 2.0e0 four times :) > > > > This data suggests that there would be value in a > > IgnorePowerOf2IntegerLiterals option. > Any chance you can hack the check run on LLVM where it doesn't report any > integer values (I'm curious about the top ten or so for floating-point > values)? Additionally, it'd be great to get similar numbers from some other > projects, like https://github.com/qt just to see how it compares (I've used > `bear` to get compile_commands.json file out of Qt before, so I would guess > that would work here). No need for the hack, I just grep for dot in my report: ``` 137 1.0 27 2.0 26 100.0 20 0.5 11 1.0f 8 1.02 7 3.0 7 1.98 7 1.5 6 .5 6 1.1 5 0.01 4 89.0f 4 4294967296.f 4 3.14159 4 2.0e0 4 10.0 3 88.0f 3 255.0 3 127.0f 3 12345.0f 3 123.45 3 0.2f 3 0.25 3 0.1f 3 0.1 2 80.0 2 710.0f 2 710.0 2 709.0f 2 6.0 2 3.14f 2 3.14 2 2.5 2 2349214918.58107 2 149.0f 2 14.5f 2 1.17549435e-38F 2 11357.0f 2 11356.0f 2 103.0f 2 0.99 2 0.9 2 0.01f 1 745.0f 1 745.0 1 7.1653228759765625e2f 1 709.0 1 7.08687663657301358331704585496e-268 1 6.62E-34 1 64.0f 1 6.02E23 1 4950.0f 1 4932.0f 1 45.0f 1 42.42 1 4.2 1 4.0 1 38.0f 1 3.7 1 323.0f 1 32.0f 1 32.0 1 3.1415 1 308.0f 1 2.718 1 2.7 1 225.0f 1 21.67 1 2.0f 1 2.088 1 .17532f 1 16445.0f 1 1.616f 1 128.0f 1 12346.0f 1 12.0f 1 1.2 1 1.1f 1 11399.0f 1 11383.0f 1 1.0L 1 1.0E+9 1 1.0E+6 1 1.0e-5f 1 1.0E+12 1 1074.0f 1 1074.0 1 1023.0f 1 1023.0 1 1.01F 1 1.01 1 10.0f 1 100. 1 0.99 1 0.8f 1 .08215f 1 0.7 1 0.6 1 0.5F
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > aaron.ballman wrote:
> > > > > > This is where I would construct an `APFloat` object from the string
> > > > > > given. As for the semantics to be used, I would recommend getting
> > > > > > it from `TargetInfo::getDoubleFormat()` on the belief that we
> > > > > > aren't going to care about precision (explained in the
> > > > > > documentation).
> > > > > Here is the problem I tried to explain last night but perhaps I
> > > > > wasn't clear enough.
> > > > >
> > > > > When we parse the input list from strings, we have to commit to one
> > > > > floating point value "semantic" - in our case single or double
> > > > > precision.
> > > > >
> > > > > When we encounter the value in the source code and it is captured by
> > > > > a matcher, it comes as either one of those values.
> > > > >
> > > > > Floats with different semantics can't be directly compared - so we
> > > > > have to maintain two distinct arrays.
> > > > >
> > > > > If we do that, rather than store APFloats and sort/compare them with
> > > > > awkward lambdas, we might as well just use the native float/double
> > > > > and be done with it more cleanly.
> > > > >When we encounter the value in the source code and it is captured by a
> > > > >matcher, it comes as either one of those values.
> > > >
> > > > It may also come in as long double or __float128, for instance, because
> > > > there are type suffixes for that.
> > > >
> > > > > Floats with different semantics can't be directly compared - so we
> > > > > have to maintain two distinct arrays.
> > > >
> > > > Yes, floats with different semantics cannot be directly compared.
> > > > That's why I said below that we should coerce the literal values.
> > > >
> > > > > If we do that, rather than store APFloats and sort/compare them with
> > > > > awkward lambdas, we might as well just use the native float/double
> > > > > and be done with it more cleanly.
> > > >
> > > > There are too many different floating-point semantics for this to be
> > > > viable, hence why coercion is a reasonable behavior.
> > > Let me see if I understood it - your proposal is: store only doubles, and
> > > when a floating-point literal is encountered in code, do not use the
> > > FloatingLiteral instance, but parse it again into a double and compare
> > > exactly. If the comparison matches - ignore it.
> > >
> > > In that case what is the value of storing APFloats with double semantics
> > > in the IgnoredValues array, instead of doubles?
> > > Let me see if I understood it - your proposal is: store only doubles, and
> > > when a floating-point literal is encountered in code, do not use the
> > > FloatingLiteral instance, but parse it again into a double and compare
> > > exactly. If the comparison matches - ignore it.
> >
> > My proposal is to use `APFloat` as the storage and comparison medium. Read
> > in strings from the configuration and convert them to an `APFloat` that has
> > double semantics. Read in literals and call `FloatLiteral::getValue()` to
> > get the `APFloat` from it, convert it to one that has double semantics as
> > needed, then perform the comparison between those two `APFloat` objects.
> >
> > > In that case what is the value of storing APFloats with double semantics
> > > in the IgnoredValues array, instead of doubles?
> >
> > Mostly that it allows us to modify or extend the check for more complicated
> > semantics in the future. Also, it's good practice to use something with
> > consistent semantic behavior across hosts and targets (comparisons between
> > numbers that cannot be precisely represented will at least be consistently
> > compared across hosts when compiling for the same target).
> >
> ok - coming right up!
> My proposal is to use APFloat as the storage and comparison medium. Read in
> strings from the configuration and convert them to an APFloat that has double
> semantics.
This is easy.
> Read in literals and call FloatLiteral::getValue() to get the APFloat from
> it,
this is easy as well,
> convert it to one that has double semantics as needed, then perform the
> comparison between those two APFloat objects.
The conversion methods in `APFloat` only produce plain-old-data-types:
```
double convertToDouble() const;
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > This is where I would construct an `APFloat` object from the
> > > > > > > > string given. As for the semantics to be used, I would
> > > > > > > > recommend getting it from `TargetInfo::getDoubleFormat()` on
> > > > > > > > the belief that we aren't going to care about precision
> > > > > > > > (explained in the documentation).
> > > > > > > Here is the problem I tried to explain last night but perhaps I
> > > > > > > wasn't clear enough.
> > > > > > >
> > > > > > > When we parse the input list from strings, we have to commit to
> > > > > > > one floating point value "semantic" - in our case single or
> > > > > > > double precision.
> > > > > > >
> > > > > > > When we encounter the value in the source code and it is captured
> > > > > > > by a matcher, it comes as either one of those values.
> > > > > > >
> > > > > > > Floats with different semantics can't be directly compared - so
> > > > > > > we have to maintain two distinct arrays.
> > > > > > >
> > > > > > > If we do that, rather than store APFloats and sort/compare them
> > > > > > > with awkward lambdas, we might as well just use the native
> > > > > > > float/double and be done with it more cleanly.
> > > > > > >When we encounter the value in the source code and it is captured
> > > > > > >by a matcher, it comes as either one of those values.
> > > > > >
> > > > > > It may also come in as long double or __float128, for instance,
> > > > > > because there are type suffixes for that.
> > > > > >
> > > > > > > Floats with different semantics can't be directly compared - so
> > > > > > > we have to maintain two distinct arrays.
> > > > > >
> > > > > > Yes, floats with different semantics cannot be directly compared.
> > > > > > That's why I said below that we should coerce the literal values.
> > > > > >
> > > > > > > If we do that, rather than store APFloats and sort/compare them
> > > > > > > with awkward lambdas, we might as well just use the native
> > > > > > > float/double and be done with it more cleanly.
> > > > > >
> > > > > > There are too many different floating-point semantics for this to
> > > > > > be viable, hence why coercion is a reasonable behavior.
> > > > > Let me see if I understood it - your proposal is: store only doubles,
> > > > > and when a floating-point literal is encountered in code, do not use
> > > > > the FloatingLiteral instance, but parse it again into a double and
> > > > > compare exactly. If the comparison matches - ignore it.
> > > > >
> > > > > In that case what is the value of storing APFloats with double
> > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > Let me see if I understood it - your proposal is: store only doubles,
> > > > > and when a floating-point literal is encountered in code, do not use
> > > > > the FloatingLiteral instance, but parse it again into a double and
> > > > > compare exactly. If the comparison matches - ignore it.
> > > >
> > > > My proposal is to use `APFloat` as the storage and comparison medium.
> > > > Read in strings from the configuration and convert them to an `APFloat`
> > > > that has double semantics. Read in literals and call
> > > > `FloatLiteral::getValue()` to get the `APFloat` from it, convert it to
> > > > one that has double semantics as needed, then perform the comparison
> > > > between those two `APFloat` objects.
> > > >
> > > > > In that case what is the value of storing APFloats with double
> > > > > semantics in the IgnoredValues array, instead of doubles?
> > > >
> > > > Mostly that it allows us to modify or extend the check for more
> > > > complicated semantics in the future. Also, it's good practice to use
> > > > something with consistent semantic behavior across hosts and targets
> > > > (comparisons between numbers that cannot be precisely represented will
> > > > at least be consistently compared across hosts when compiling for the
> > > > same target).
> > > >
> > > ok - coming right up!
> > > My proposal is to use APFloat as the storage and comparison medium. Read
> > > in strings from the configuration and convert them to an APFloat that has
> > > double semantics.
> >
> > This is easy.
> >
> > >
Re: r338049 - [OPENMP] What's new for OpenMP in clang.
I just noticed that UsersManual says: "Clang supports all OpenMP 3.1 directives and clauses." Maybe this should link to OpenMPSupport? On 2018-07-26 19:53, Alexey Bataev via cfe-commits wrote: Author: abataev Date: Thu Jul 26 10:53:45 2018 New Revision: 338049 URL: http://llvm.org/viewvc/llvm-project?rev=338049&view=rev Log: [OPENMP] What's new for OpenMP in clang. Updated ReleaseNotes + Status of the OpenMP support in clang. Modified: cfe/trunk/docs/OpenMPSupport.rst cfe/trunk/docs/ReleaseNotes.rst Modified: cfe/trunk/docs/OpenMPSupport.rst URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/OpenMPSupport.rst?rev=338049&r1=338048&r2=338049&view=diff == --- cfe/trunk/docs/OpenMPSupport.rst (original) +++ cfe/trunk/docs/OpenMPSupport.rst Thu Jul 26 10:53:45 2018 @@ -10,13 +10,15 @@ .. role:: partial .. role:: good +.. contents:: + :local: + == OpenMP Support == -Clang fully supports OpenMP 3.1 + some elements of OpenMP 4.5. Clang supports offloading to X86_64, AArch64 and PPC64[LE] devices. -Support for Cuda devices is not ready yet. -The status of major OpenMP 4.5 features support in Clang. +Clang fully supports OpenMP 4.5. Clang supports offloading to X86_64, AArch64, +PPC64[LE] and has `basic support for Cuda devices`_. Standalone directives = @@ -35,7 +37,7 @@ Standalone directives * #pragma omp target: :good:`Complete`. -* #pragma omp declare target: :partial:`Partial`. No full codegen support. +* #pragma omp declare target: :good:`Complete`. * #pragma omp teams: :good:`Complete`. @@ -64,5 +66,66 @@ Combined directives * #pragma omp target teams distribute parallel for [simd]: :good:`Complete`. -Clang does not support any constructs/updates from upcoming OpenMP 5.0 except for `reduction`-based clauses in the `task` and `target`-based directives. -In addition, the LLVM OpenMP runtime `libomp` supports the OpenMP Tools Interface (OMPT) on x86, x86_64, AArch64, and PPC64 on Linux, Windows, and mac OS. +Clang does not support any constructs/updates from upcoming OpenMP 5.0 except +for `reduction`-based clauses in the `task` and `target`-based directives. + +In addition, the LLVM OpenMP runtime `libomp` supports the OpenMP Tools +Interface (OMPT) on x86, x86_64, AArch64, and PPC64 on Linux, Windows, and mac OS. +ows, and mac OS. + +.. _basic support for Cuda devices: + +Cuda devices support + + +Directives execution modes +-- + +Clang code generation for target regions supports two modes: the SPMD and +non-SPMD modes. Clang chooses one of these two modes automatically based on the +way directives and clauses on those directives are used. The SPMD mode uses a +simplified set of runtime functions thus increasing performance at the cost of +supporting some OpenMP features. The non-SPMD mode is the most generic mode and +supports all currently available OpenMP features. The compiler will always +attempt to use the SPMD mode wherever possible. SPMD mode will not be used if: + + - The target region contains an `if()` clause that refers to a `parallel` + directive. + + - The target region contains a `parallel` directive with a `num_threads()` + clause. + + - The target region contains user code (other than OpenMP-specific + directives) in between the `target` and the `parallel` directives. + +Data-sharing modes +-- + +Clang supports two data-sharing models for Cuda devices: `Generic` and `Cuda` +modes. The default mode is `Generic`. `Cuda` mode can give an additional +performance and can be activated using the `-fopenmp-cuda-mode` flag. In +`Generic` mode all local variables that can be shared in the parallel regions +are stored in the global memory. In `Cuda` mode local variables are not shared +between the threads and it is user responsibility to share the required data +between the threads in the parallel regions. + +Features not supported or with limited support for Cuda devices +--- + +- Reductions across the teams are not supported yet. + +- Cancellation constructs are not supported. + +- Doacross loop nest is not supported. + +- User-defined reductions are supported only for trivial types. + +- Nested parallelism: inner parallel regions are executed sequentially. + +- Static linking of libraries containing device code is not supported yet. + +- Automatic translation of math functions in target regions to device-specific + math functions is not implemented yet. + +- Debug information for OpenMP target regions is not supported yet. + Modified: cfe/trunk/docs/ReleaseNotes.rst URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=338049&r1=338048&r2=338049&view=diff == ---
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > 0x8000- wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > This is where I would construct an `APFloat` object from the
> > > > > > > > > string given. As for the semantics to be used, I would
> > > > > > > > > recommend getting it from `TargetInfo::getDoubleFormat()` on
> > > > > > > > > the belief that we aren't going to care about precision
> > > > > > > > > (explained in the documentation).
> > > > > > > > Here is the problem I tried to explain last night but perhaps I
> > > > > > > > wasn't clear enough.
> > > > > > > >
> > > > > > > > When we parse the input list from strings, we have to commit to
> > > > > > > > one floating point value "semantic" - in our case single or
> > > > > > > > double precision.
> > > > > > > >
> > > > > > > > When we encounter the value in the source code and it is
> > > > > > > > captured by a matcher, it comes as either one of those values.
> > > > > > > >
> > > > > > > > Floats with different semantics can't be directly compared - so
> > > > > > > > we have to maintain two distinct arrays.
> > > > > > > >
> > > > > > > > If we do that, rather than store APFloats and sort/compare them
> > > > > > > > with awkward lambdas, we might as well just use the native
> > > > > > > > float/double and be done with it more cleanly.
> > > > > > > >When we encounter the value in the source code and it is
> > > > > > > >captured by a matcher, it comes as either one of those values.
> > > > > > >
> > > > > > > It may also come in as long double or __float128, for instance,
> > > > > > > because there are type suffixes for that.
> > > > > > >
> > > > > > > > Floats with different semantics can't be directly compared - so
> > > > > > > > we have to maintain two distinct arrays.
> > > > > > >
> > > > > > > Yes, floats with different semantics cannot be directly compared.
> > > > > > > That's why I said below that we should coerce the literal values.
> > > > > > >
> > > > > > > > If we do that, rather than store APFloats and sort/compare them
> > > > > > > > with awkward lambdas, we might as well just use the native
> > > > > > > > float/double and be done with it more cleanly.
> > > > > > >
> > > > > > > There are too many different floating-point semantics for this to
> > > > > > > be viable, hence why coercion is a reasonable behavior.
> > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > doubles, and when a floating-point literal is encountered in code,
> > > > > > do not use the FloatingLiteral instance, but parse it again into a
> > > > > > double and compare exactly. If the comparison matches - ignore it.
> > > > > >
> > > > > > In that case what is the value of storing APFloats with double
> > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > doubles, and when a floating-point literal is encountered in code,
> > > > > > do not use the FloatingLiteral instance, but parse it again into a
> > > > > > double and compare exactly. If the comparison matches - ignore it.
> > > > >
> > > > > My proposal is to use `APFloat` as the storage and comparison medium.
> > > > > Read in strings from the configuration and convert them to an
> > > > > `APFloat` that has double semantics. Read in literals and call
> > > > > `FloatLiteral::getValue()` to get the `APFloat` from it, convert it
> > > > > to one that has double semantics as needed, then perform the
> > > > > comparison between those two `APFloat` objects.
> > > > >
> > > > > > In that case what is the value of storing APFloats with double
> > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > >
> > > > > Mostly that it allows us to modify or extend the check for more
> > > > > complicated semantics in the future. Also, it's good practice to use
> > > > > something with consistent semantic behavior across hosts and targets
> > > > > (comparisons between numbers that cannot be precisely represented
> > > > > will at least be consistently compared across hosts when compiling
> > > > > for the same target).
> > > > >
> > > > ok - coming right up!
> > > > My proposal is to use APFloat as the
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- updated this revision to Diff 157889.
0x8000- added a comment.
Add a flag to ignore all powers of two integral values.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
Files:
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MagicNumbersCheck.cpp
clang-tidy/readability/MagicNumbersCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-magic-numbers.rst
test/clang-tidy/readability-magic-numbers.cpp
Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN: {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}, \
+// RUN: {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template
+struct ValueBucket {
+ T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+ return param * param;
+}
+
+void BuggyFunction() {
+ int BadLocalInt = 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ (void)IntSquarer(7);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int LocalArray[15];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ for (int ii = 0; ii < 22; ++ii)
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ }
+
+ ValueBucket Bucket;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+ TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int getValue() const;
+
+private:
+ int oneMember = 9;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int anotherMember;
+
+ int yetAnotherMember;
+
+ const int oneConstant = 2;
+
+ const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+ if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+ return -3; // FILENOTFOUND
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+ const int GoodLocalIntConstant = 43;
+
+ (void)IntSquarer(GoodLocalIntConstant);
+
+ int LocalArray[INT_MACRO];
+
+ ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100, 65536};
+
+float GrandfatheredFloatValues[] = {3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4};
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+ STARTER,
+ ALPHA = 3,
+ BETA = 1 << 5,
+};
+
+cons
Re: r338049 - [OPENMP] What's new for OpenMP in clang.
Yes, that would be good Best regards, Alexey Bataev > 29 июля 2018 г., в 12:41, Jonas Hahnfeld via cfe-commits > написал(а): > > I just noticed that UsersManual says: "Clang supports all OpenMP 3.1 > directives and clauses." Maybe this should link to OpenMPSupport? > >> On 2018-07-26 19:53, Alexey Bataev via cfe-commits wrote: >> Author: abataev >> Date: Thu Jul 26 10:53:45 2018 >> New Revision: 338049 >> URL: http://llvm.org/viewvc/llvm-project?rev=338049&view=rev >> Log: >> [OPENMP] What's new for OpenMP in clang. >> Updated ReleaseNotes + Status of the OpenMP support in clang. >> Modified: >>cfe/trunk/docs/OpenMPSupport.rst >>cfe/trunk/docs/ReleaseNotes.rst >> Modified: cfe/trunk/docs/OpenMPSupport.rst >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/OpenMPSupport.rst?rev=338049&r1=338048&r2=338049&view=diff >> == >> --- cfe/trunk/docs/OpenMPSupport.rst (original) >> +++ cfe/trunk/docs/OpenMPSupport.rst Thu Jul 26 10:53:45 2018 >> @@ -10,13 +10,15 @@ >> .. role:: partial >> .. role:: good >> +.. contents:: >> + :local: >> + >> == >> OpenMP Support >> == >> -Clang fully supports OpenMP 3.1 + some elements of OpenMP 4.5. Clang >> supports offloading to X86_64, AArch64 and PPC64[LE] devices. >> -Support for Cuda devices is not ready yet. >> -The status of major OpenMP 4.5 features support in Clang. >> +Clang fully supports OpenMP 4.5. Clang supports offloading to X86_64, >> AArch64, >> +PPC64[LE] and has `basic support for Cuda devices`_. >> Standalone directives >> = >> @@ -35,7 +37,7 @@ Standalone directives >> * #pragma omp target: :good:`Complete`. >> -* #pragma omp declare target: :partial:`Partial`. No full codegen support. >> +* #pragma omp declare target: :good:`Complete`. >> * #pragma omp teams: :good:`Complete`. >> @@ -64,5 +66,66 @@ Combined directives >> * #pragma omp target teams distribute parallel for [simd]: :good:`Complete`. >> -Clang does not support any constructs/updates from upcoming OpenMP >> 5.0 except for `reduction`-based clauses in the `task` and >> `target`-based directives. >> -In addition, the LLVM OpenMP runtime `libomp` supports the OpenMP >> Tools Interface (OMPT) on x86, x86_64, AArch64, and PPC64 on Linux, >> Windows, and mac OS. >> +Clang does not support any constructs/updates from upcoming OpenMP 5.0 >> except >> +for `reduction`-based clauses in the `task` and `target`-based directives. >> + >> +In addition, the LLVM OpenMP runtime `libomp` supports the OpenMP Tools >> +Interface (OMPT) on x86, x86_64, AArch64, and PPC64 on Linux, >> Windows, and mac OS. >> +ows, and mac OS. >> + >> +.. _basic support for Cuda devices: >> + >> +Cuda devices support >> + >> + >> +Directives execution modes >> +-- >> + >> +Clang code generation for target regions supports two modes: the SPMD and >> +non-SPMD modes. Clang chooses one of these two modes automatically based on >> the >> +way directives and clauses on those directives are used. The SPMD mode uses >> a >> +simplified set of runtime functions thus increasing performance at the cost >> of >> +supporting some OpenMP features. The non-SPMD mode is the most generic mode >> and >> +supports all currently available OpenMP features. The compiler will always >> +attempt to use the SPMD mode wherever possible. SPMD mode will not be used >> if: >> + >> + - The target region contains an `if()` clause that refers to a `parallel` >> + directive. >> + >> + - The target region contains a `parallel` directive with a >> `num_threads()` >> + clause. >> + >> + - The target region contains user code (other than OpenMP-specific >> + directives) in between the `target` and the `parallel` directives. >> + >> +Data-sharing modes >> +-- >> + >> +Clang supports two data-sharing models for Cuda devices: `Generic` and >> `Cuda` >> +modes. The default mode is `Generic`. `Cuda` mode can give an additional >> +performance and can be activated using the `-fopenmp-cuda-mode` flag. In >> +`Generic` mode all local variables that can be shared in the parallel >> regions >> +are stored in the global memory. In `Cuda` mode local variables are not >> shared >> +between the threads and it is user responsibility to share the required data >> +between the threads in the parallel regions. >> + >> +Features not supported or with limited support for Cuda devices >> +--- >> + >> +- Reductions across the teams are not supported yet. >> + >> +- Cancellation constructs are not supported. >> + >> +- Doacross loop nest is not supported. >> + >> +- User-defined reductions are supported only for trivial types. >> + >> +- Nested parallelism: inner parallel regions are executed sequentially. >> + >> +- Static linking of libraries containing device code is not supported yet. >> +
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > 0x8000- wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > This is where I would construct an `APFloat` object from
> > > > > > > > > > the string given. As for the semantics to be used, I would
> > > > > > > > > > recommend getting it from `TargetInfo::getDoubleFormat()`
> > > > > > > > > > on the belief that we aren't going to care about precision
> > > > > > > > > > (explained in the documentation).
> > > > > > > > > Here is the problem I tried to explain last night but perhaps
> > > > > > > > > I wasn't clear enough.
> > > > > > > > >
> > > > > > > > > When we parse the input list from strings, we have to commit
> > > > > > > > > to one floating point value "semantic" - in our case single
> > > > > > > > > or double precision.
> > > > > > > > >
> > > > > > > > > When we encounter the value in the source code and it is
> > > > > > > > > captured by a matcher, it comes as either one of those values.
> > > > > > > > >
> > > > > > > > > Floats with different semantics can't be directly compared -
> > > > > > > > > so we have to maintain two distinct arrays.
> > > > > > > > >
> > > > > > > > > If we do that, rather than store APFloats and sort/compare
> > > > > > > > > them with awkward lambdas, we might as well just use the
> > > > > > > > > native float/double and be done with it more cleanly.
> > > > > > > > >When we encounter the value in the source code and it is
> > > > > > > > >captured by a matcher, it comes as either one of those values.
> > > > > > > >
> > > > > > > > It may also come in as long double or __float128, for instance,
> > > > > > > > because there are type suffixes for that.
> > > > > > > >
> > > > > > > > > Floats with different semantics can't be directly compared -
> > > > > > > > > so we have to maintain two distinct arrays.
> > > > > > > >
> > > > > > > > Yes, floats with different semantics cannot be directly
> > > > > > > > compared. That's why I said below that we should coerce the
> > > > > > > > literal values.
> > > > > > > >
> > > > > > > > > If we do that, rather than store APFloats and sort/compare
> > > > > > > > > them with awkward lambdas, we might as well just use the
> > > > > > > > > native float/double and be done with it more cleanly.
> > > > > > > >
> > > > > > > > There are too many different floating-point semantics for this
> > > > > > > > to be viable, hence why coercion is a reasonable behavior.
> > > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > > doubles, and when a floating-point literal is encountered in
> > > > > > > code, do not use the FloatingLiteral instance, but parse it again
> > > > > > > into a double and compare exactly. If the comparison matches -
> > > > > > > ignore it.
> > > > > > >
> > > > > > > In that case what is the value of storing APFloats with double
> > > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > > doubles, and when a floating-point literal is encountered in
> > > > > > > code, do not use the FloatingLiteral instance, but parse it again
> > > > > > > into a double and compare exactly. If the comparison matches -
> > > > > > > ignore it.
> > > > > >
> > > > > > My proposal is to use `APFloat` as the storage and comparison
> > > > > > medium. Read in strings from the configuration and convert them to
> > > > > > an `APFloat` that has double semantics. Read in literals and call
> > > > > > `FloatLiteral::getValue()` to get the `APFloat` from it, convert it
> > > > > > to one that has double semantics as needed, then perform the
> > > > > > comparison between those two `APFloat` objects.
> > > > > >
> > > > > > > In that case what is the value of storing APFloats with double
> > > > > > > semantics in the IgnoredValues array, instead of doubles?
> > > > > >
> > > > > > Mostly that it allows us to modify or extend the check for more
> > > > > > complicated semantics in the future. Also, it's good practice to
> > > > > > use something with consistent semantic behavior across hosts and
> > > > > > targets (comparisons between numbers that cannot be pre
[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.
rwols created this revision.
rwols added reviewers: sammccall, ilya-biryukov, ioeric.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay.
Some LSP clients (e.g. Sublime Text) assume that the label and the completion
trigger have a common
prefix. This assumption is broken by prepending a space or dot in front of the
label, depending
on wether we're inserting an `#include <...>` text edit or not. This change
allows to clear the
prepended characters in the label.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49967
Files:
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,13 +12,13 @@
#include "Path.h"
#include "Trace.h"
#include "index/SymbolYAML.h"
+#include "clang/Basic/Version.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
-#include "clang/Basic/Version.h"
#include
#include
#include
@@ -157,6 +157,14 @@
llvm::cl::init(clangd::CodeCompleteOptions().ShowOrigins),
llvm::cl::Hidden);
+static llvm::cl::opt NoHeaderInsertDecorators(
+"no-header-insert-decorators",
+llvm::cl::desc(
+"Do not prepend a circular dot or space before the completion "
+"label, depending on wether "
+"an include line will be inserted or not."),
+llvm::cl::init(false));
+
static llvm::cl::opt YamlSymbolFile(
"yaml-symbol-file",
llvm::cl::desc(
@@ -276,6 +284,10 @@
CCOpts.Limit = LimitResults;
CCOpts.BundleOverloads = CompletionStyle != Detailed;
CCOpts.ShowOrigins = ShowOrigins;
+ if (NoHeaderInsertDecorators) {
+CCOpts.IncludeIndicator.Insert.clear();
+CCOpts.IncludeIndicator.NoInsert.clear();
+ }
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,13 +12,13 @@
#include "Path.h"
#include "Trace.h"
#include "index/SymbolYAML.h"
+#include "clang/Basic/Version.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
-#include "clang/Basic/Version.h"
#include
#include
#include
@@ -157,6 +157,14 @@
llvm::cl::init(clangd::CodeCompleteOptions().ShowOrigins),
llvm::cl::Hidden);
+static llvm::cl::opt NoHeaderInsertDecorators(
+"no-header-insert-decorators",
+llvm::cl::desc(
+"Do not prepend a circular dot or space before the completion "
+"label, depending on wether "
+"an include line will be inserted or not."),
+llvm::cl::init(false));
+
static llvm::cl::opt YamlSymbolFile(
"yaml-symbol-file",
llvm::cl::desc(
@@ -276,6 +284,10 @@
CCOpts.Limit = LimitResults;
CCOpts.BundleOverloads = CompletionStyle != Detailed;
CCOpts.ShowOrigins = ShowOrigins;
+ if (NoHeaderInsertDecorators) {
+CCOpts.IncludeIndicator.Insert.clear();
+CCOpts.IncludeIndicator.NoInsert.clear();
+ }
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added a comment. Top 40 magic numbers in https://github.com/qt/qtbase 4859 2 2901 3 1855 4 985 5 968 8 605 6 600 7 439 16 432 10 363 356 32 241 1.0f 217 12 209 255 207 100 205 9 205 20 204 50 177 0.5 174 15 162 0x2 144 24 140 0x80 135 11 127 256 113 14 110 0xff 101 1.0 99 64 99 200 96 13 86 30 84 1000 68 18 66 150 62 127 62 0xFF 58 19 58 0.05f 57 128 Top 40 floating point magic numbers in https://github.com/qt/qtbase 241 1.0f 177 0.5 101 1.0 58 0.05f 44 2.0 42 0.5f 31 10.0 28 30.0 24 20.0 22 60.0 20 100.0 19 0.8 19 0.25 17 0.2 16 1000.0 14 1.224744871 14 100. 13 25.0 13 0.1 12 90.0 12 40.0 12 0.707106781 12 0.30 12 0.20 11 80.0 11 6.0 11 50.0 11 2.0f 11 0.75 11 0.66f 11 0.1f 10 6.28 10 5.0 10 4.0 10 1.414213562 9 360.0 9 25.4 9 2.54 8 70.0 8 55.0 Top 40 magic numbers in https://github.com/facebook/rocksdb 2131 2 896 3 859 4 858 10 685 100 678 1024 600 8 445 5 323 1000 244 20 231 301 227 200 223 6 209 16 189 7 154 1 131 100 119 10 111 30 105 256 104 32 103 5U 103 50 94 128 91 64 89 60 88 3U 85 2U 84 500 72 4U 67 9 65 300 63 13 59 0xff 57 6U 52 4096 52 24 52 12 51 600 50 10U Top 40 floating point numbers in rocksdb: 37 100.0 30 1.0 27 0.5 24 0.001 12 1048576.0 12 0.25 11 1.1 8 50.0 8 1.5 8 1.0 5 .3 5 .1 5 0.8 4 99.99 4 99.9 4 2.0 4 1.048576 4 100.0f 4 0.9 4 0.75 4 0.69 4 0.02 4 0.1 3 100.0 3 0.4 3 0.1 2 0.7 2 0.6 2 0.45 1 8.0 1 5.6 1 40.2 1 40.1 1 3.25 1 2.0 1 2. 1 116.2 1 116.1 1 110.5e-4 1 1024.0 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
aaron.ballman wrote:
> aaron.ballman wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > 0x8000- wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > This is where I would construct an `APFloat` object
> > > > > > > > > > > > from the string given. As for the semantics to be used,
> > > > > > > > > > > > I would recommend getting it from
> > > > > > > > > > > > `TargetInfo::getDoubleFormat()` on the belief that we
> > > > > > > > > > > > aren't going to care about precision (explained in the
> > > > > > > > > > > > documentation).
> > > > > > > > > > > Here is the problem I tried to explain last night but
> > > > > > > > > > > perhaps I wasn't clear enough.
> > > > > > > > > > >
> > > > > > > > > > > When we parse the input list from strings, we have to
> > > > > > > > > > > commit to one floating point value "semantic" - in our
> > > > > > > > > > > case single or double precision.
> > > > > > > > > > >
> > > > > > > > > > > When we encounter the value in the source code and it is
> > > > > > > > > > > captured by a matcher, it comes as either one of those
> > > > > > > > > > > values.
> > > > > > > > > > >
> > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > >
> > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > sort/compare them with awkward lambdas, we might as well
> > > > > > > > > > > just use the native float/double and be done with it more
> > > > > > > > > > > cleanly.
> > > > > > > > > > >When we encounter the value in the source code and it is
> > > > > > > > > > >captured by a matcher, it comes as either one of those
> > > > > > > > > > >values.
> > > > > > > > > >
> > > > > > > > > > It may also come in as long double or __float128, for
> > > > > > > > > > instance, because there are type suffixes for that.
> > > > > > > > > >
> > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > >
> > > > > > > > > > Yes, floats with different semantics cannot be directly
> > > > > > > > > > compared. That's why I said below that we should coerce the
> > > > > > > > > > literal values.
> > > > > > > > > >
> > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > sort/compare them with awkward lambdas, we might as well
> > > > > > > > > > > just use the native float/double and be done with it more
> > > > > > > > > > > cleanly.
> > > > > > > > > >
> > > > > > > > > > There are too many different floating-point semantics for
> > > > > > > > > > this to be viable, hence why coercion is a reasonable
> > > > > > > > > > behavior.
> > > > > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > > > > doubles, and when a floating-point literal is encountered in
> > > > > > > > > code, do not use the FloatingLiteral instance, but parse it
> > > > > > > > > again into a double and compare exactly. If the comparison
> > > > > > > > > matches - ignore it.
> > > > > > > > >
> > > > > > > > > In that case what is the value of storing APFloats with
> > > > > > > > > double semantics in the IgnoredValues array, instead of
> > > > > > > > > doubles?
> > > > > > > > > Let me see if I understood it - your proposal is: store only
> > > > > > > > > doubles, and when a floating-point literal is encountered in
> > > > > > > > > code, do not use the FloatingLiteral instance, but parse it
> > > > > > > > > again into a double and compare exactly. If the comparison
> > > > > > > > > matches - ignore it.
> > > > > > > >
> > > > > > > > My proposal is to use `APFloat` as the storage and comparison
> > > > > > > > medium. Read in strings from the configuration and convert them
> > > > > > > > to an `APFloat` that has double semantics. Read in literals and
> > > > > > > > call `FloatLiteral::getValue()` to get the `APFloat` from it,
> > > > > > > > convert it to one that has double semantics as needed, then
> > > > > > > > perform the comparison between those two `APFloat` objects
[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Makes sense, thanks! Comment at: clangd/tool/ClangdMain.cpp:161 +static llvm::cl::opt NoHeaderInsertDecorators( +"no-header-insert-decorators", +llvm::cl::desc( Please invert the sense of this to avoid a double negative: "header-insert-decorators" with default true. Consider insert->insertion, because currently is not clear whether to read as (header (insert decorators)) or as ((header insert) decorators). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49967 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> 0x8000- wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > 0x8000- wrote:
> > > > > 0x8000- wrote:
> > > > > > 0x8000- wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > This is where I would construct an `APFloat` object
> > > > > > > > > > > > > > from the string given. As for the semantics to be
> > > > > > > > > > > > > > used, I would recommend getting it from
> > > > > > > > > > > > > > `TargetInfo::getDoubleFormat()` on the belief that
> > > > > > > > > > > > > > we aren't going to care about precision (explained
> > > > > > > > > > > > > > in the documentation).
> > > > > > > > > > > > > Here is the problem I tried to explain last night but
> > > > > > > > > > > > > perhaps I wasn't clear enough.
> > > > > > > > > > > > >
> > > > > > > > > > > > > When we parse the input list from strings, we have to
> > > > > > > > > > > > > commit to one floating point value "semantic" - in
> > > > > > > > > > > > > our case single or double precision.
> > > > > > > > > > > > >
> > > > > > > > > > > > > When we encounter the value in the source code and it
> > > > > > > > > > > > > is captured by a matcher, it comes as either one of
> > > > > > > > > > > > > those values.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > > > >
> > > > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > > > sort/compare them with awkward lambdas, we might as
> > > > > > > > > > > > > well just use the native float/double and be done
> > > > > > > > > > > > > with it more cleanly.
> > > > > > > > > > > > >When we encounter the value in the source code and it
> > > > > > > > > > > > >is captured by a matcher, it comes as either one of
> > > > > > > > > > > > >those values.
> > > > > > > > > > > >
> > > > > > > > > > > > It may also come in as long double or __float128, for
> > > > > > > > > > > > instance, because there are type suffixes for that.
> > > > > > > > > > > >
> > > > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > > > compared - so we have to maintain two distinct arrays.
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, floats with different semantics cannot be directly
> > > > > > > > > > > > compared. That's why I said below that we should coerce
> > > > > > > > > > > > the literal values.
> > > > > > > > > > > >
> > > > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > > > sort/compare them with awkward lambdas, we might as
> > > > > > > > > > > > > well just use the native float/double and be done
> > > > > > > > > > > > > with it more cleanly.
> > > > > > > > > > > >
> > > > > > > > > > > > There are too many different floating-point semantics
> > > > > > > > > > > > for this to be viable, hence why coercion is a
> > > > > > > > > > > > reasonable behavior.
> > > > > > > > > > > Let me see if I understood it - your proposal is: store
> > > > > > > > > > > only doubles, and when a floating-point literal is
> > > > > > > > > > > encountered in code, do not use the FloatingLiteral
> > > > > > > > > > > instance, but parse it again into a double and compare
> > > > > > > > > > > exactly. If the comparison matches - ignore it.
> > > > > > > > > > >
> > > > > > > > > > > In that case what is the value of storing APFloats with
> > > > > > > > > > > double semantics in the IgnoredValues array, instead of
> > > > > > > > > > > doubles?
> > > > > > > > > > > Let me see if I understood it - your proposal is: store
> > > > > > > > > > > only doubles, and when a floating-point literal is
> > > > > > > > > > > encountered in code, do not use the FloatingLiteral
> > > > > > > > > > > instance, but parse it again into a double and compare
> > > > > > > > > > > exactly. If the comparison matches - ignore it.
> > > > > > > > > >
> > > > > > > > > > My proposal is to use `APFloat` as the storage and
> > > > > > > > > > comparison medium. Read in strings from the con
[PATCH] D48098: clang-format-diff: Make it work with python3 too
MarcoFalke added a comment. In https://reviews.llvm.org/D48098#1174771, @krasimir wrote: > MarcoFalke: do you need someone to submit this for you? Yes, I'd appreciate any help on how to submit this for merge. https://reviews.llvm.org/D48098 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
aaron.ballman added a comment.
In https://reviews.llvm.org/D49114#1179652, @0x8000- wrote:
> Top 40 magic numbers in https://github.com/qt/qtbase
>
> 4859 2
> 2901 3
> 1855 4
>985 5
>968 8
>605 6
>600 7
>439 16
>432 10
>363
>356 32
>241 1.0f
>217 12
>209 255
>207 100
>205 9
>205 20
>204 50
>177 0.5
>174 15
>162 0x2
>144 24
>140 0x80
>135 11
>127 256
>113 14
>110 0xff
>101 1.0
> 99 64
> 99 200
> 96 13
> 86 30
> 84 1000
> 68 18
> 66 150
> 62 127
> 62 0xFF
> 58 19
> 58 0.05f
> 57 128
>
>
> Top 40 floating point magic numbers in https://github.com/qt/qtbase
>
> 241 1.0f
> 177 0.5
> 101 1.0
>58 0.05f
>44 2.0
>42 0.5f
>31 10.0
>28 30.0
>24 20.0
>22 60.0
>20 100.0
>19 0.8
>19 0.25
>17 0.2
>16 1000.0
>14 1.224744871
>14 100.
>13 25.0
>13 0.1
>12 90.0
>12 40.0
>12 0.707106781
>12 0.30
>12 0.20
>11 80.0
>11 6.0
>11 50.0
>11 2.0f
>11 0.75
>11 0.66f
>11 0.1f
>10 6.28
>10 5.0
>10 4.0
>10 1.414213562
> 9 360.0
> 9 25.4
> 9 2.54
> 8 70.0
> 8 55.0
>
>
> Top 40 magic numbers in https://github.com/facebook/rocksdb
>
> 2131 2
>896 3
>859 4
>858 10
>685 100
>678 1024
>600 8
>445 5
>323 1000
>244 20
>231 301
>227 200
>223 6
>209 16
>189 7
>154 1
>131 100
>119 10
>111 30
>105 256
>104 32
>103 5U
>103 50
> 94 128
> 91 64
> 89 60
> 88 3U
> 85 2U
> 84 500
> 72 4U
> 67 9
> 65 300
> 63 13
> 59 0xff
> 57 6U
> 52 4096
> 52 24
> 52 12
> 51 600
> 50 10U
>
>
> Top 40 floating point numbers in rocksdb:
>
> 37 100.0
> 30 1.0
> 27 0.5
> 24 0.001
> 12 1048576.0
> 12 0.25
> 11 1.1
>8 50.0
>8 1.5
>8 1.0
>5 .3
>5 .1
>5 0.8
>4 99.99
>4 99.9
>4 2.0
>4 1.048576
>4 100.0f
>4 0.9
>4 0.75
>4 0.69
>4 0.02
>4 0.1
>3 100.0
>3 0.4
>3 0.1
>2 0.7
>2 0.6
>2 0.45
>1 8.0
>1 5.6
>1 40.2
>1 40.1
>1 3.25
>1 2.0
>1 2.
>1 116.2
>1 116.1
>1 110.5e-4
>1 1024.0
>
Awesome, thank you for this! Based on this, I think the integer list should
also include 2, 3, and 4 as defaults -- those show up a lot more than I'd have
expected. As for floating-point values, 1.0 certainly jumps out at me, but none
of the rest seem particularly common. What do you think?
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
0x8000- wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > 0x8000- wrote:
> > > > > > 0x8000- wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > 0x8000- wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > This is where I would construct an `APFloat`
> > > > > > > > > > > > > > > object from the string given. As for the
> > > > > > > > > > > > > > > semantics to be used, I would recommend getting
> > > > > > > > > > > > > > > it from `TargetInfo::getDoubleFormat()` on the
> > > > > > > > > > > > > > > belief that we aren't going to care about
> > > > > > > > > > > > > > > precision (explained in the documentation).
> > > > > > > > > > > > > > Here is the problem I tried to explain last night
> > > > > > > > > > > > > > but perhaps I wasn't clear enough.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When we parse the input list from strings, we have
> > > > > > > > > > > > > > to commit to one floating point value "semantic" -
> > > > > > > > > > > > > > in our case single or double precision.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When we encounter the value in the source code and
> > > > > > > > > > > > > > it is captured by a matcher, it comes as either one
> > > > > > > > > > > > > > of those values.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > > > > compared - so we have to maintain two dist
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added a comment. > Based on this, I think the integer list should also include 2, 3, and 4 as > defaults -- those show up a lot more than I'd have expected. As for > floating-point values, 1.0 certainly jumps out at me, but none of the rest > seem particularly common. What do you think? I'm good with 0, 1, 2, 3, 4 for integers and 0.0, 1.0 and also 100.0 (used to compute percentages) for floating point values. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.
rwols updated this revision to Diff 157894.
rwols added a comment.
Avoid double negative for command line option
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49967
Files:
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,13 +12,13 @@
#include "Path.h"
#include "Trace.h"
#include "index/SymbolYAML.h"
+#include "clang/Basic/Version.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
-#include "clang/Basic/Version.h"
#include
#include
#include
@@ -157,6 +157,13 @@
llvm::cl::init(clangd::CodeCompleteOptions().ShowOrigins),
llvm::cl::Hidden);
+static llvm::cl::opt HeaderInsertionDecorators(
+"header-insertion-decorators",
+llvm::cl::desc("Prepend a circular dot or space before the completion "
+ "label, depending on wether "
+ "an include line will be inserted or not."),
+llvm::cl::init(true));
+
static llvm::cl::opt YamlSymbolFile(
"yaml-symbol-file",
llvm::cl::desc(
@@ -276,6 +283,10 @@
CCOpts.Limit = LimitResults;
CCOpts.BundleOverloads = CompletionStyle != Detailed;
CCOpts.ShowOrigins = ShowOrigins;
+ if (!HeaderInsertionDecorators) {
+CCOpts.IncludeIndicator.Insert.clear();
+CCOpts.IncludeIndicator.NoInsert.clear();
+ }
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,13 +12,13 @@
#include "Path.h"
#include "Trace.h"
#include "index/SymbolYAML.h"
+#include "clang/Basic/Version.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
-#include "clang/Basic/Version.h"
#include
#include
#include
@@ -157,6 +157,13 @@
llvm::cl::init(clangd::CodeCompleteOptions().ShowOrigins),
llvm::cl::Hidden);
+static llvm::cl::opt HeaderInsertionDecorators(
+"header-insertion-decorators",
+llvm::cl::desc("Prepend a circular dot or space before the completion "
+ "label, depending on wether "
+ "an include line will be inserted or not."),
+llvm::cl::init(true));
+
static llvm::cl::opt YamlSymbolFile(
"yaml-symbol-file",
llvm::cl::desc(
@@ -276,6 +283,10 @@
CCOpts.Limit = LimitResults;
CCOpts.BundleOverloads = CompletionStyle != Detailed;
CCOpts.ShowOrigins = ShowOrigins;
+ if (!HeaderInsertionDecorators) {
+CCOpts.IncludeIndicator.Insert.clear();
+CCOpts.IncludeIndicator.NoInsert.clear();
+ }
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r338223 - [clangd] Add command-line option
Author: rwols
Date: Sun Jul 29 12:12:42 2018
New Revision: 338223
URL: http://llvm.org/viewvc/llvm-project?rev=338223&view=rev
Log:
[clangd] Add command-line option
to suppress the space and the circular dot prepended in a completion label.
Modified:
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
Modified: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp?rev=338223&r1=338222&r2=338223&view=diff
==
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp (original)
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp Sun Jul 29 12:12:42 2018
@@ -12,13 +12,13 @@
#include "Path.h"
#include "Trace.h"
#include "index/SymbolYAML.h"
+#include "clang/Basic/Version.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
-#include "clang/Basic/Version.h"
#include
#include
#include
@@ -157,6 +157,13 @@ static llvm::cl::opt
llvm::cl::init(clangd::CodeCompleteOptions().ShowOrigins),
llvm::cl::Hidden);
+static llvm::cl::opt HeaderInsertionDecorators(
+"header-insertion-decorators",
+llvm::cl::desc("Prepend a circular dot or space before the completion "
+ "label, depending on wether "
+ "an include line will be inserted or not."),
+llvm::cl::init(true));
+
static llvm::cl::opt YamlSymbolFile(
"yaml-symbol-file",
llvm::cl::desc(
@@ -276,6 +283,10 @@ int main(int argc, char *argv[]) {
CCOpts.Limit = LimitResults;
CCOpts.BundleOverloads = CompletionStyle != Detailed;
CCOpts.ShowOrigins = ShowOrigins;
+ if (!HeaderInsertionDecorators) {
+CCOpts.IncludeIndicator.Insert.clear();
+CCOpts.IncludeIndicator.NoInsert.clear();
+ }
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49967: [clangd] Add command-line option to suppress the space and the circular dot prepended in a completion label.
rwols closed this revision. rwols added a comment. Oh... I should have used `arc land` instead of `svn commit`. Here's the commit: https://reviews.llvm.org/rCTE338223 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49967 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- added inline comments.
Comment at: clang-tidy/readability/MagicNumbersCheck.cpp:76-86
+ IgnoredFloatingPointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ IgnoredDoublePointValues.reserve(IgnoredFloatingPointValuesInput.size());
+ for (const auto &InputValue : IgnoredFloatingPointValuesInput) {
+llvm::APFloat FloatValue(llvm::APFloat::IEEEsingle());
+FloatValue.convertFromString(InputValue, DefaultRoundingMode);
+IgnoredFloatingPointValues.push_back(FloatValue.convertToFloat());
+
aaron.ballman wrote:
> 0x8000- wrote:
> > 0x8000- wrote:
> > > 0x8000- wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > 0x8000- wrote:
> > > > > > > 0x8000- wrote:
> > > > > > > > 0x8000- wrote:
> > > > > > > > > 0x8000- wrote:
> > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > 0x8000- wrote:
> > > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > > This is where I would construct an `APFloat`
> > > > > > > > > > > > > > > > object from the string given. As for the
> > > > > > > > > > > > > > > > semantics to be used, I would recommend getting
> > > > > > > > > > > > > > > > it from `TargetInfo::getDoubleFormat()` on the
> > > > > > > > > > > > > > > > belief that we aren't going to care about
> > > > > > > > > > > > > > > > precision (explained in the documentation).
> > > > > > > > > > > > > > > Here is the problem I tried to explain last night
> > > > > > > > > > > > > > > but perhaps I wasn't clear enough.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When we parse the input list from strings, we
> > > > > > > > > > > > > > > have to commit to one floating point value
> > > > > > > > > > > > > > > "semantic" - in our case single or double
> > > > > > > > > > > > > > > precision.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > When we encounter the value in the source code
> > > > > > > > > > > > > > > and it is captured by a matcher, it comes as
> > > > > > > > > > > > > > > either one of those values.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > > > > > compared - so we have to maintain two distinct
> > > > > > > > > > > > > > > arrays.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > > > > > sort/compare them with awkward lambdas, we might
> > > > > > > > > > > > > > > as well just use the native float/double and be
> > > > > > > > > > > > > > > done with it more cleanly.
> > > > > > > > > > > > > > >When we encounter the value in the source code and
> > > > > > > > > > > > > > >it is captured by a matcher, it comes as either
> > > > > > > > > > > > > > >one of those values.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It may also come in as long double or __float128,
> > > > > > > > > > > > > > for instance, because there are type suffixes for
> > > > > > > > > > > > > > that.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Floats with different semantics can't be directly
> > > > > > > > > > > > > > > compared - so we have to maintain two distinct
> > > > > > > > > > > > > > > arrays.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Yes, floats with different semantics cannot be
> > > > > > > > > > > > > > directly compared. That's why I said below that we
> > > > > > > > > > > > > > should coerce the literal values.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > If we do that, rather than store APFloats and
> > > > > > > > > > > > > > > sort/compare them with awkward lambdas, we might
> > > > > > > > > > > > > > > as well just use the native float/double and be
> > > > > > > > > > > > > > > done with it more cleanly.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > There are too many different floating-point
> > > > > > > > > > > > > > semantics for this to be viable, hence why coercion
> > > > > > > > > > > > > > is a reasonable behavior.
> > > > > > > > > > > > > Let me see if I understood it - your proposal is:
> > > > > > > > > > > > > store only doubles, and when a floating-point literal
> > > > > > > > > > > > > is encountered in code, do not use the
> > > > > > > > > > > > > FloatingLiteral instance, but parse it again into a
> > > > > > > > > > > > > double and compare exactly. If the comparison matches
> > > > > > > > > > > > > - ignore it.
> > > > > > > > > > > > >
> > > > > > > > > > > > > In that case what is the value of storing APFloats
> > > > > > > > > > > > > with double semantics in the IgnoredValues array,
> > > > > > > > > > > > > instead of doubles?
> > > > > > > > > > > > > Let me see if I understood it - your propos
[PATCH] D49114: [clang-tidy] Add a check for "magic numbers"
0x8000- updated this revision to Diff 157899.
0x8000- added a comment.
Update the list of magic values ignored by default.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49114
Files:
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MagicNumbersCheck.cpp
clang-tidy/readability/MagicNumbersCheck.h
clang-tidy/readability/ReadabilityTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-avoid-magic-numbers.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/readability-magic-numbers.rst
test/clang-tidy/readability-magic-numbers.cpp
Index: test/clang-tidy/readability-magic-numbers.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-magic-numbers.cpp
@@ -0,0 +1,195 @@
+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
+// RUN: {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0"}, \
+// RUN: {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
+// RUN: --
+
+template
+struct ValueBucket {
+ T value[V];
+};
+
+int BadGlobalInt = 5;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int IntSquarer(int param) {
+ return param * param;
+}
+
+void BuggyFunction() {
+ int BadLocalInt = 6;
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ (void)IntSquarer(7);
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int LocalArray[15];
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 15 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ for (int ii = 0; ii < 22; ++ii)
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 22 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ {
+LocalArray[ii] = 3 * ii;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+ }
+
+ ValueBucket Bucket;
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 66 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+class TwoIntContainer {
+public:
+ TwoIntContainer(int val) : anotherMember(val * val), yetAnotherMember(6), anotherConstant(val + val) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:73: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int getValue() const;
+
+private:
+ int oneMember = 9;
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 9 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+ int anotherMember;
+
+ int yetAnotherMember;
+
+ const int oneConstant = 2;
+
+ const int anotherConstant;
+};
+
+int ValueArray[] = {3, 5};
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+float FloatPiVariable = 3.1415926535f;
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3.1415926535f is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+double DoublePiVariable = 6.283185307;
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: 6.283185307 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+
+int getAnswer() {
+ if (ValueArray[0] < ValueArray[1])
+return ValueArray[1];
+
+ return -3; // FILENOTFOUND
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
+
+/*
+ * Clean code
+ */
+
+#define INT_MACRO 5
+
+const int GoodGlobalIntConstant = 42;
+
+constexpr int AlsoGoodGlobalIntConstant = 42;
+
+int InitializedByMacro = INT_MACRO;
+
+void SolidFunction() {
+ const int GoodLocalIntConstant = 43;
+
+ (void)IntSquarer(GoodLocalIntConstant);
+
+ int LocalArray[INT_MACRO];
+
+ ValueBucket Bucket;
+}
+
+const int ConstValueArray[] = {7, 9};
+
+const int ConstValueArray2D[2][2] = {{7, 9}, {13, 15}};
+
+/*
+ * no warnings for ignored values (specified in the configuration above)
+ */
+int GrandfatheredIntegerValues[] = {0, 1, 2, 10, 100, -1, -10, -100, 65536};
+
+float GrandfatheredFloatValues[] = {3.14f, 3.14, 2.71828, 2.71828f, -1.01E2, 1E4};
+
+/*
+ * no warnings for enums
+ */
+enum Smorgasbord {
+ STARTER,
+ ALPHA = 3,
+ BETA = 1 << 5,
+};
+
+const fl
[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen
Charusso updated this revision to Diff 157916.
Charusso added a comment.
Excuse me for the huge patch, but what I can done wrongly, I did, so I have
made tons of revision.
I would like to show the current direction of the checker. Currently it has too
much overlapping function, so please don't go too deep into the core.
Major fixes in every function:
- Equal length of `strlen(src)` is now handled.
- It can read out the length of the destination buffer.
- If the length increases in the passed argument, it also increases the
destination buffer (if overflow is looks like to possible).
Major fixes in `memcpy()` and `memmove()`:
- It can decide which function is the best in performance and safety. (The last
implementation was rely on C++11 version, which was a huge mistake.)
- Handles cases where the destination is `unsigned char` or `signed char`,
which cannot be passed to any string handler function. (It haven't checked the
type so that made wrong fix-its.)
Minor fixes:
- Removed all memory allocation matchers in general, but the current functions
behave the same to increase the buffer length.
- Now the test files' `RUN` command working well. (CheckOptions was wrong.)
Problematic:
1. It allows all custom memory allocation function. Sometimes the read buffer
size is not the size parameter.
2. May it is not a good idea to heuristically read and increase buffer lengths.
3. If the new function transformed from `memcpy()` to `strncpy()`, the checker
adds the null terminator expression in the next line, like `dest[length] =
'\0'`, which is looks like a quite big injection.
https://reviews.llvm.org/D45050
Files:
clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tidy/bugprone/CMakeLists.txt
clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
clang-tidy/bugprone/NotNullTerminatedResultCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c
test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c
test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp
test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c
test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c
test/clang-tidy/bugprone-not-null-terminated-result-strlen.c
test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp
test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
Index: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c++11
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+template
+errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+
+template
+wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t);
+
+template
+errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *);
+errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *);
+
+template
+wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *);
+wchar_t *wcscpy(wchar_t *, const wchar_t *);
+
+errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t);
+
+
+//===--===//
+// wmemcpy() - destination array tests
+//===--===//
+
+void bad_wmemcpy_known_dest(const wchar_t *src) {
+ wchar_t dest01[13];
+ wmemcpy(dest01, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wchar_t dest01[14];
+ // CHECK-FIXES-NEXT: wcscpy_s(dest01, src);
+}
+
+void good_wmemcpy_known_dest(const wchar_t *src) {
+ wchar_t dst01[14];
+ wcscpy_s(dst01, src);
+}
+
+//===--===//
+// wmemcpy() - length tests
+//===--===//
+
+void bad_wmemcpy_full_source_length(const wchar_t *src) {
+ wchar_t dest20[13];
+ wmemcpy(dest20, src, wcslen(src));
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: wchar_t dest20[14];
+ // CHECK-FIXES-NEXT: wcscpy_s(dest20, src);
+}
+
+void good_wmemcpy_full_source_length(const wchar_t *src) {
+ wchar_t dst20[14];
+ wcsc
[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen
Charusso marked 2 inline comments as done.
Charusso added inline comments.
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:377-381
+FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), ") + 1");
+Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix;
+ } else {
+const auto InsertPlusOne =
+FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " + 1");
lebedev.ri wrote:
> The fixits are incorrect.
> Increment by `int(1)` can result in overflow, which is then extended to
> `size_t`
> It should be `+ 1UL`.
> https://godbolt.org/g/4nQiek
Could you show me a true example where it causes a problem? I think it is too
rare to rewrite all code as you mentioned.
Comment at:
test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c:158-180
+void bad_memset_1(char *dest, const char *src) {
+ int length = getLengthWithInc(src);
+ memset(dest, '-', length);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset'
is not null-terminated [bugprone-not-null-terminated-result]
+ // CHECK-FIXES: memset(dest, '-', length - 1);
+}
+
lebedev.ri wrote:
> ??
> These look like false-negatives.
> How do you make an assumption about `dest` buffer from `src` string length?
My idea here is you do not want to set your null terminator to anything else,
because then the result is not null-terminated.
https://reviews.llvm.org/D45050
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
