[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); aaron.ballman wrote: > `const auto *` Why do we need this change here? If I understand correctly, with `const auto*` we also need change initializer to `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. But I don't understand why we need this. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK updated this revision to Diff 172515.
ZaMaZaN4iK added a comment.
1. Fix indentation in test file
2. Return back capitalization for the checker description
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
test/Analysis/enum-cast-out-of-range.cpp
www/analyzer/alpha_checks.html
Index: www/analyzer/alpha_checks.html
===
--- www/analyzer/alpha_checks.html
+++ www/analyzer/alpha_checks.html
@@ -348,6 +348,24 @@
}
+
+alpha.cplusplus.EnumCastOutOfRange
+(C++)
+ Check for integer to enumeration casts that could result in undefined values.
+
+
+
+enum TestEnum {
+ A = 0
+};
+
+void foo() {
+ TestEnum t = static_cast(-1);
+ // warn: the value provided to the cast expression is not in
+ the valid range of values for the enum
+}
+
+
alpha.cplusplus.InvalidatedIterator
Index: test/Analysis/enum-cast-out-of-range.cpp
===
--- /dev/null
+++ test/Analysis/enum-cast-out-of-range.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN: -std=c++11 -verify %s
+
+enum unscoped_unspecified_t {
+ unscoped_unspecified_0 = -4,
+ unscoped_unspecified_1,
+ unscoped_unspecified_2 = 1,
+ unscoped_unspecified_3,
+ unscoped_unspecified_4 = 4
+};
+
+enum unscoped_specified_t : int {
+ unscoped_specified_0 = -4,
+ unscoped_specified_1,
+ unscoped_specified_2 = 1,
+ unscoped_specified_3,
+ unscoped_specified_4 = 4
+};
+
+enum class scoped_unspecified_t {
+ scoped_unspecified_0 = -4,
+ scoped_unspecified_1,
+ scoped_unspecified_2 = 1,
+ scoped_unspecified_3,
+ scoped_unspecified_4 = 4
+};
+
+enum class scoped_specified_t : int {
+ scoped_specified_0 = -4,
+ scoped_specified_1,
+ scoped_specified_2 = 1,
+ scoped_specified_3,
+ scoped_specified_4 = 4
+};
+
+struct S {
+ unscoped_unspecified_t E : 5;
+};
+
+void unscopedUnspecified() {
+ unscoped_unspecified_t InvalidBeforeRangeBegin = static_cast(-5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidNegativeValue1 = static_cast(-4); // OK.
+ unscoped_unspecified_t ValidNegativeValue2 = static_cast(-3); // OK.
+ unscoped_unspecified_t InvalidInsideRange1 = static_cast(-2); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t InvalidInsideRange2 = static_cast(-1); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t InvalidInsideRange3 = static_cast(0); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidPositiveValue1 = static_cast(1); // OK.
+ unscoped_unspecified_t ValidPositiveValue2 = static_cast(2); // OK.
+ unscoped_unspecified_t InvalidInsideRange4 = static_cast(3); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidPositiveValue3 = static_cast(4); // OK.
+ unscoped_unspecified_t InvalidAfterRangeEnd = static_cast(5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+}
+
+void unscopedSpecified() {
+ unscoped_specified_t InvalidBeforeRangeBegin = static_cast(-5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidNegativeValue1 = static_cast(-4); // OK.
+ unscoped_specified_t ValidNegativeValue2 = static_cast(-3); // OK.
+ unscoped_specified_t InvalidInsideRange1 = static_cast(-2); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t InvalidInsideRange2 = static_cast(-1); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t InvalidInsideRange3 = static_cast(0); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidPositiveValue1 = static_cast(1); // OK.
+ unscoped_specified_t ValidPositiveValue2 = static_cast(2); // OK.
+ unscoped_specified_t InvalidInsideRange4 = static_cast(3); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidPositiveValue3 = static_cast(4); // OK.
+ unscoped_specified_t InvalidAfterRangeEnd = static_cast(5); // expected-warning {{the value provided to the cast expression is
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
lebedev.ri added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); ZaMaZaN4iK wrote: > aaron.ballman wrote: > > `const auto *` > Why do we need this change here? If I understand correctly, with `const > auto*` we also need change initializer to > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. But > I don't understand why we need this. Is `ValueToCastOptional` a pointer, a reference, or just an actual `DefinedOrUnknownSVal`? I can't tell. (sidenote: would be great to have a clang-tidy check for this.) https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); lebedev.ri wrote: > ZaMaZaN4iK wrote: > > aaron.ballman wrote: > > > `const auto *` > > Why do we need this change here? If I understand correctly, with `const > > auto*` we also need change initializer to > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > But I don't understand why we need this. > Is `ValueToCastOptional` a pointer, a reference, or just an actual > `DefinedOrUnknownSVal`? I can't tell. > (sidenote: would be great to have a clang-tidy check for this.) `ValueToCastOptional` is `llvm::Optional` https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
lebedev.ri added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); ZaMaZaN4iK wrote: > lebedev.ri wrote: > > ZaMaZaN4iK wrote: > > > aaron.ballman wrote: > > > > `const auto *` > > > Why do we need this change here? If I understand correctly, with `const > > > auto*` we also need change initializer to > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > But I don't understand why we need this. > > Is `ValueToCastOptional` a pointer, a reference, or just an actual > > `DefinedOrUnknownSVal`? I can't tell. > > (sidenote: would be great to have a clang-tidy check for this.) > `ValueToCastOptional` is `llvm::Optional` See, all my guesses were wrong. That is why it should not be `auto` at all. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
ztamas marked 10 inline comments as done. ztamas added a comment. Mark all handled inline comments as Done. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); lebedev.ri wrote: > ZaMaZaN4iK wrote: > > lebedev.ri wrote: > > > ZaMaZaN4iK wrote: > > > > aaron.ballman wrote: > > > > > `const auto *` > > > > Why do we need this change here? If I understand correctly, with `const > > > > auto*` we also need change initializer to > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > But I don't understand why we need this. > > > Is `ValueToCastOptional` a pointer, a reference, or just an actual > > > `DefinedOrUnknownSVal`? I can't tell. > > > (sidenote: would be great to have a clang-tidy check for this.) > > `ValueToCastOptional` is `llvm::Optional` > See, all my guesses were wrong. That is why it should not be `auto` at all. I don't agree with you for this case. Honestly it's like a yet another holywar question. If we are talking only about this case - here you can see `getAs` part of the expression. this means clearly (at least for me) that we get something like `DefinedOrUnknownSVal`. What we get? I just press hotkey in my favourite IDE/text editor and see that `getAs` returns `llvm::Optional`. From my point of view it's clear enough here. If we are talking more generally about question "When should we use `auto` at all? " - we can talk, but not here, I think :) https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
lebedev.ri added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); ZaMaZaN4iK wrote: > lebedev.ri wrote: > > ZaMaZaN4iK wrote: > > > lebedev.ri wrote: > > > > ZaMaZaN4iK wrote: > > > > > aaron.ballman wrote: > > > > > > `const auto *` > > > > > Why do we need this change here? If I understand correctly, with > > > > > `const auto*` we also need change initializer to > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > But I don't understand why we need this. > > > > Is `ValueToCastOptional` a pointer, a reference, or just an actual > > > > `DefinedOrUnknownSVal`? I can't tell. > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > `ValueToCastOptional` is `llvm::Optional` > > See, all my guesses were wrong. That is why it should not be `auto` at all. > I don't agree with you for this case. Honestly it's like a yet another > holywar question. If we are talking only about this case - here you can see > `getAs` part of the expression. this means clearly (at > least for me) that we get something like `DefinedOrUnknownSVal`. What we get? > I just press hotkey in my favourite IDE/text editor and see that `getAs` > returns `llvm::Optional`. From my point of view it's > clear enough here. > > If we are talking more generally about question "When should we use `auto` at > all? " - we can talk, but not here, I think :) https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable comes to mind. > What we get? I just press hotkey in my favourite IDE/text editor and see that > getAs returns llvm::Optional Which hotkey do i need to press to see this here, in the phabricator? This really shouldn't be `auto`, if you have to explain that in the variable's name, justify it in review comments. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); lebedev.ri wrote: > ZaMaZaN4iK wrote: > > lebedev.ri wrote: > > > ZaMaZaN4iK wrote: > > > > lebedev.ri wrote: > > > > > ZaMaZaN4iK wrote: > > > > > > aaron.ballman wrote: > > > > > > > `const auto *` > > > > > > Why do we need this change here? If I understand correctly, with > > > > > > `const auto*` we also need change initializer to > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > But I don't understand why we need this. > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an actual > > > > > `DefinedOrUnknownSVal`? I can't tell. > > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > > `ValueToCastOptional` is `llvm::Optional` > > > See, all my guesses were wrong. That is why it should not be `auto` at > > > all. > > I don't agree with you for this case. Honestly it's like a yet another > > holywar question. If we are talking only about this case - here you can see > > `getAs` part of the expression. this means clearly > > (at least for me) that we get something like `DefinedOrUnknownSVal`. What > > we get? I just press hotkey in my favourite IDE/text editor and see that > > `getAs` returns `llvm::Optional`. From my point of > > view it's clear enough here. > > > > If we are talking more generally about question "When should we use `auto` > > at all? " - we can talk, but not here, I think :) > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > comes to mind. > > What we get? I just press hotkey in my favourite IDE/text editor and see > > that getAs returns llvm::Optional > Which hotkey do i need to press to see this here, in the phabricator? > > This really shouldn't be `auto`, if you have to explain that in the > variable's name, justify it in review comments. Ok, didn't know about such LLVM coding standard. Of course, with this information I will fix using `auto` here. Thank you. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
ZaMaZaN4iK updated this revision to Diff 172516.
ZaMaZaN4iK added a comment.
Fix using `auto` in case where it leads to worse readability
https://reviews.llvm.org/D33672
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
test/Analysis/enum-cast-out-of-range.cpp
www/analyzer/alpha_checks.html
Index: www/analyzer/alpha_checks.html
===
--- www/analyzer/alpha_checks.html
+++ www/analyzer/alpha_checks.html
@@ -348,6 +348,24 @@
}
+
+alpha.cplusplus.EnumCastOutOfRange
+(C++)
+ Check for integer to enumeration casts that could result in undefined values.
+
+
+
+enum TestEnum {
+ A = 0
+};
+
+void foo() {
+ TestEnum t = static_cast(-1);
+ // warn: the value provided to the cast expression is not in
+ the valid range of values for the enum
+}
+
+
alpha.cplusplus.InvalidatedIterator
Index: test/Analysis/enum-cast-out-of-range.cpp
===
--- /dev/null
+++ test/Analysis/enum-cast-out-of-range.cpp
@@ -0,0 +1,192 @@
+// RUN: %clang_analyze_cc1 \
+// RUN: -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN: -std=c++11 -verify %s
+
+enum unscoped_unspecified_t {
+ unscoped_unspecified_0 = -4,
+ unscoped_unspecified_1,
+ unscoped_unspecified_2 = 1,
+ unscoped_unspecified_3,
+ unscoped_unspecified_4 = 4
+};
+
+enum unscoped_specified_t : int {
+ unscoped_specified_0 = -4,
+ unscoped_specified_1,
+ unscoped_specified_2 = 1,
+ unscoped_specified_3,
+ unscoped_specified_4 = 4
+};
+
+enum class scoped_unspecified_t {
+ scoped_unspecified_0 = -4,
+ scoped_unspecified_1,
+ scoped_unspecified_2 = 1,
+ scoped_unspecified_3,
+ scoped_unspecified_4 = 4
+};
+
+enum class scoped_specified_t : int {
+ scoped_specified_0 = -4,
+ scoped_specified_1,
+ scoped_specified_2 = 1,
+ scoped_specified_3,
+ scoped_specified_4 = 4
+};
+
+struct S {
+ unscoped_unspecified_t E : 5;
+};
+
+void unscopedUnspecified() {
+ unscoped_unspecified_t InvalidBeforeRangeBegin = static_cast(-5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidNegativeValue1 = static_cast(-4); // OK.
+ unscoped_unspecified_t ValidNegativeValue2 = static_cast(-3); // OK.
+ unscoped_unspecified_t InvalidInsideRange1 = static_cast(-2); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t InvalidInsideRange2 = static_cast(-1); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t InvalidInsideRange3 = static_cast(0); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidPositiveValue1 = static_cast(1); // OK.
+ unscoped_unspecified_t ValidPositiveValue2 = static_cast(2); // OK.
+ unscoped_unspecified_t InvalidInsideRange4 = static_cast(3); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_unspecified_t ValidPositiveValue3 = static_cast(4); // OK.
+ unscoped_unspecified_t InvalidAfterRangeEnd = static_cast(5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+}
+
+void unscopedSpecified() {
+ unscoped_specified_t InvalidBeforeRangeBegin = static_cast(-5); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidNegativeValue1 = static_cast(-4); // OK.
+ unscoped_specified_t ValidNegativeValue2 = static_cast(-3); // OK.
+ unscoped_specified_t InvalidInsideRange1 = static_cast(-2); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t InvalidInsideRange2 = static_cast(-1); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t InvalidInsideRange3 = static_cast(0); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidPositiveValue1 = static_cast(1); // OK.
+ unscoped_specified_t ValidPositiveValue2 = static_cast(2); // OK.
+ unscoped_specified_t InvalidInsideRange4 = static_cast(3); // expected-warning {{the value provided to the cast expression is not in the valid range of values for the enum}}
+ unscoped_specified_t ValidPositiveValue3 = static_cast(4); // OK.
+ unscoped_specified_t InvalidAfterRangeEnd = static_cast(5); // expected-warning {{the value provided to the cast expression is not in the valid range of val
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); ZaMaZaN4iK wrote: > lebedev.ri wrote: > > ZaMaZaN4iK wrote: > > > lebedev.ri wrote: > > > > ZaMaZaN4iK wrote: > > > > > lebedev.ri wrote: > > > > > > ZaMaZaN4iK wrote: > > > > > > > aaron.ballman wrote: > > > > > > > > `const auto *` > > > > > > > Why do we need this change here? If I understand correctly, with > > > > > > > `const auto*` we also need change initializer to > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > But I don't understand why we need this. > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an actual > > > > > > `DefinedOrUnknownSVal`? I can't tell. > > > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > > > `ValueToCastOptional` is `llvm::Optional` > > > > See, all my guesses were wrong. That is why it should not be `auto` at > > > > all. > > > I don't agree with you for this case. Honestly it's like a yet another > > > holywar question. If we are talking only about this case - here you can > > > see `getAs` part of the expression. this means > > > clearly (at least for me) that we get something like > > > `DefinedOrUnknownSVal`. What we get? I just press hotkey in my favourite > > > IDE/text editor and see that `getAs` returns > > > `llvm::Optional`. From my point of view it's clear > > > enough here. > > > > > > If we are talking more generally about question "When should we use > > > `auto` at all? " - we can talk, but not here, I think :) > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > comes to mind. > > > What we get? I just press hotkey in my favourite IDE/text editor and see > > > that getAs returns llvm::Optional > > Which hotkey do i need to press to see this here, in the phabricator? > > > > This really shouldn't be `auto`, if you have to explain that in the > > variable's name, justify it in review comments. > Ok, didn't know about such LLVM coding standard. Of course, with this > information I will fix using `auto` here. Thank you. Actually, I disagree. In the Static Analyzer we use `auto` if the return type is in the name of the expression, and `getAs` may fail, so it returns an `Optional`. In the case where a nullptr may be returned to signal failure, `auto *` is used, so I believe that `auto` is appropriate here. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); Szelethus wrote: > ZaMaZaN4iK wrote: > > lebedev.ri wrote: > > > ZaMaZaN4iK wrote: > > > > lebedev.ri wrote: > > > > > ZaMaZaN4iK wrote: > > > > > > lebedev.ri wrote: > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > aaron.ballman wrote: > > > > > > > > > `const auto *` > > > > > > > > Why do we need this change here? If I understand correctly, > > > > > > > > with `const auto*` we also need change initializer to > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > But I don't understand why we need this. > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an > > > > > > > actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > > > > `ValueToCastOptional` is `llvm::Optional` > > > > > See, all my guesses were wrong. That is why it should not be `auto` > > > > > at all. > > > > I don't agree with you for this case. Honestly it's like a yet another > > > > holywar question. If we are talking only about this case - here you can > > > > see `getAs` part of the expression. this means > > > > clearly (at least for me) that we get something like > > > > `DefinedOrUnknownSVal`. What we get? I just press hotkey in my > > > > favourite IDE/text editor and see that `getAs` returns > > > > `llvm::Optional`. From my point of view it's > > > > clear enough here. > > > > > > > > If we are talking more generally about question "When should we use > > > > `auto` at all? " - we can talk, but not here, I think :) > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > comes to mind. > > > > What we get? I just press hotkey in my favourite IDE/text editor and > > > > see that getAs returns llvm::Optional > > > Which hotkey do i need to press to see this here, in the phabricator? > > > > > > This really shouldn't be `auto`, if you have to explain that in the > > > variable's name, justify it in review comments. > > Ok, didn't know about such LLVM coding standard. Of course, with this > > information I will fix using `auto` here. Thank you. > Actually, I disagree. In the Static Analyzer we use `auto` if the return type > is in the name of the expression, and `getAs` may fail, so it returns an > `Optional`. In the case where a nullptr may be returned to signal failure, > `auto *` is used, so I believe that `auto` is appropriate here. But don't change it back now, it doesn't matter a whole lot :D https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53974: [clang-tidy] new check: bugprone-too-small-loop-variable
JonasToth added a comment.
Regarding the warning discussion: It is ok to have this check here in
clang-tidy first and let it mature. Once we can handle all kind of loops and do
not produce false positives this logic can move into the frontend.
But in my opinion the warning-version should handle all loops. Until then we
can happily have this here :)
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:45
+
+ // We need to catch only those comparisons which contain any integer cast
+ StatementMatcher LoopVarConversionMatcher =
missing full stop.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:51
+
+ // We are interested in only those cases when the condition is a variable
+ // value (not const, enum, etc.)
same
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:60
+
+ // We use the loop increment expression only to make sure we found the right
+ // loop variable
same.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:82
+
+/// Returns the positive part of the integer width for an integer type
+unsigned calcPositiveBits(const ASTContext &Context,
same, other places too, but i won't mark them anymore.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:100
+ // Inside a binary operator ignore casting caused by constant values
+ // (constants, macros defiened values, enums, literals)
+ // We are interested in variable values' positive bits
`marco defined` is outdated.
I think the sentence should be improved. Maybe `Ignore casting cause by
constant values inside a binary operator, e.g. from ... .`?
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:120
+if (RHSEIsConstantValue && LHSEIsConstantValue)
+ return 0; // Avoid false positives produced by two constant values
+else if (RHSEIsConstantValue)
I think that comment should be before the `if` to be consistent with other
comment positions, and missing full stop.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:122
+else if (RHSEIsConstantValue)
+ CondExprPosBits = calcPositiveBits(Context, LHSEType);
+else if (LHSEIsConstantValue)
you can utilize early return for all these cases.
Please don't use `if`-`else` then, because no `return` after else-rule.
Comment at: clang-tidy/bugprone/TooSmallLoopVariableCheck.cpp:142
+ if (LoopVar->getType() != LoopIncrement->getType())
+return; // We matched the loop variable incorrectly
+
Does this try to ensure a precondition? Then it should become an assertion
instead.
Please adjust the comment like above (punctuation, position)
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:6
+
+Detects those ``for`` loops which has a loop variable with a "too small" type
+which means this type can't represent all values which are part of the
iteration
Disclaimer: english isn't my mother tongue and its not perfect :)
`which has` -> `that have`? Sounds better to me.
Comment at: docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:10
+
+ .. code-block:: c++
+
the `.. code-block:: c++` is usually not indended, only the code itself.
Comment at: test/clang-tidy/bugprone-too-small-loop-variable.cpp:5
+
+void voidBadForLoop() {
+ for (int i = 0; i < size(); ++i) {
please add tests for `range-for` loops to ensure the implicitly generated loop
does not generate any false positives or the like.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53974
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
JonasToth added inline comments.
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84
"cppcoreguidelines-c-copy-assignment-signature");
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-c-arrays");
lebedev.ri wrote:
> JonasToth wrote:
> > please conserve the alphabetical order here
> Sorted all the `CheckFactories.registerCheck<>();` lines.
the `avoid-*` checks seem to be displaced now.
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:44
+ unless(anyOf(hasParent(varDecl(isExternC())),
+ hasAncestor(functionDecl(isExternC())
+ .bind("typeloc"),
lebedev.ri wrote:
> JonasToth wrote:
> > What about struct-decls that are externC?
> Hm, what is a `struct-decl`?
we chatted in irc, but for reference
```
extern "C" {
struct Foo {
};
}
```
Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:3
+
+int a[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use
std::array<> instead
what about variable-length arrays? I think they should be diagnosed as well,
but plain replacement with `std::array` would be incorrect, more a
`std::vector` would fit.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
JonasToth added inline comments.
Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:44
+ unless(anyOf(hasParent(varDecl(isExternC())),
+ hasAncestor(functionDecl(isExternC())
+ .bind("typeloc"),
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > What about struct-decls that are externC?
> > Hm, what is a `struct-decl`?
> we chatted in irc, but for reference
> ```
> extern "C" {
> struct Foo {
>
> };
> }
> ```
you can mark all comments as done here, as they are implemented.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
lebedev.ri marked 4 inline comments as done.
lebedev.ri added a comment.
(only comments, patch to follow)
Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84
"cppcoreguidelines-c-copy-assignment-signature");
+CheckFactories.registerCheck(
+"cppcoreguidelines-avoid-c-arrays");
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > please conserve the alphabetical order here
> > Sorted all the `CheckFactories.registerCheck<>();` lines.
> the `avoid-*` checks seem to be displaced now.
How it should be sorted? By the new check name? Or the internal class name?
If it is not the latter, then sorting is really problematic as it can't be
automatized.
Comment at: test/clang-tidy/modernize-avoid-c-arrays.cpp:10-11
+void foo() {
+ int c[b[0]];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays,
use std::array<> instead
+}
This is VLA.
Note that VLA are C99, they (thankfully!) don't exist in C++ standard at all.
Looks like `variableArrayType()` should single-out this, so let's see..
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53771
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346095 - [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
Author: szelethus
Date: Sun Nov 4 05:59:44 2018
New Revision: 346095
URL: http://llvm.org/viewvc/llvm-project?rev=346095&view=rev
Log:
[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and
primitive expansion
This patch adds a couple new functions to acquire the macro's name, and also
expands it, although it doesn't expand the arguments, as seen from the test
files
Differential Revision: https://reviews.llvm.org/D52794
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=346095&r1=346094&r2=346095&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Sun Nov 4 05:59:44
2018
@@ -16,6 +16,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/TokenConcatenation.h"
#include "clang/Rewrite/Core/HTMLRewrite.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
@@ -400,12 +401,6 @@ void PlistPrinter::ReportNote(raw_ostrea
// Static function definitions.
//===--===//
-static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
- const Preprocessor &PP) {
- // TODO: Implement macro expansion.
- return { "", "" };
-}
-
/// Print coverage information to output stream {@code o}.
/// May modify the used list of files {@code Fids} by inserting new ones.
static void printCoverage(const PathDiagnostic *D,
@@ -721,3 +716,196 @@ void PlistDiagnostics::FlushDiagnosticsI
// Finish.
o << "\n";
}
+
+//===--===//
+// Declarations of helper functions and data structures for expanding macros.
+//===--===//
+
+namespace {
+
+struct MacroNameAndInfo {
+ std::string Name;
+ const MacroInfo *MI = nullptr;
+
+ MacroNameAndInfo(std::string N, const MacroInfo *MI)
+: Name(std::move(N)), MI(MI) {}
+};
+
+/// Helper class for printing tokens.
+class TokenPrinter {
+ llvm::raw_ostream &OS;
+ const Preprocessor &PP;
+
+ Token PrevTok, PrevPrevTok;
+ TokenConcatenation ConcatInfo;
+
+public:
+ TokenPrinter(llvm::raw_ostream &OS, const Preprocessor &PP)
+: OS(OS), PP(PP), ConcatInfo(PP) {
+PrevTok.setKind(tok::unknown);
+PrevPrevTok.setKind(tok::unknown);
+ }
+
+ void printToken(const Token &Tok);
+};
+
+} // end of anonymous namespace
+
+static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
+ SourceLocation MacroLoc,
+ const Preprocessor &PP);
+
+/// Retrieves the name of the macro and its MacroInfo.
+static MacroNameAndInfo getMacroNameAndInfo(SourceLocation ExpanLoc,
+const Preprocessor &PP);
+
+/// Retrieves the ')' token that matches '(' \p It points to.
+static MacroInfo::tokens_iterator getMatchingRParen(
+MacroInfo::tokens_iterator It,
+MacroInfo::tokens_iterator End);
+
+/// Retrieves the macro info for \p II refers to at \p Loc. This is important
+/// because macros can be redefined or undefined.
+static const MacroInfo *getMacroInfoForLocation(const Preprocessor &PP,
+const SourceManager &SM,
+const IdentifierInfo *II,
+SourceLocation Loc);
+
+//===--===//
+// Definitions of helper functions and methods for expanding macros.
+//===--===//
+
+static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
+ const Preprocessor &PP) {
+
+ llvm::SmallString<200> ExpansionBuf;
+ llvm::raw_svector_ostream OS(ExpansionBuf);
+ TokenPrinter Printer(OS, PP);
+ return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP), OS.str() };
+}
+
+static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
+ SourceLocation MacroLoc,
+ const Preprocessor &PP) {
+
+ const SourceManager &SM = PP.getSourceManager();
+
+ MacroNameAndInfo Info = getMacroNameAndInfo(SM.getExpansionLoc(MacroLoc),
PP);
+ const MacroInfo *MI =
[PATCH] D52794: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
This revision was automatically updated to reflect the committed changes. Closed by commit rL346095: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and… (authored by Szelethus, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D52794?vs=172459&id=172519#toc Repository: rL LLVM https://reviews.llvm.org/D52794 Files: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp Index: cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist === --- cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist +++ cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist @@ -143,8 +143,8 @@ col3 file0 - name - expansion + nameSET_PTR_VAR_TO_NULL + expansionptr = 0 descriptionDereference of null pointer (loaded from variable 'ptr') @@ -312,8 +312,8 @@ col3 file0 - name - expansion + nameSET_PTR_VAR_TO_NULL_WITH_NESTED_MACRO + expansionptr =0 descriptionDereference of null pointer (loaded from variable 'ptr') @@ -342,10 +342,1047 @@ + + path + + + kindcontrol + edges + + +start + + + line58 + col3 + file0 + + + line58 + col5 + file0 + + +end + + + line59 + col3 + file0 + + + line59 + col9 + file0 + + + + + + + kindevent + location + + line59 + col3 + file0 + + ranges + + + + line59 + col3 + file0 + + + line59 + col15 + file0 + + + + depth0 + extended_message + Calling 'setToNull' + message + Calling 'setToNull' + + + kindevent + location + + line50 + col1 + file0 + + depth1 + extended_message + Entered call from 'functionLikeMacroTest' + message + Entered call from 'functionLikeMacroTest' + + + kindcontrol + edges + + +start + + + line50 + col1 + file0 + + + line50 + col4 + file0 + + +end + + + line51 + col3 + file0 + + + line51 + col3 + file0 + + + + + + + kindevent + location + + line51 + col3 + file0 + + ranges + + + + line51 + col3 + file0 + + + line51 + col17 + file0 + + + + depth1 + extended_message + Null pointer value stored to 'ptr' + message + Null pointer value stored to 'ptr' + + + kindevent + location + + line59 + col3 + file0 + + ranges + + + + line59 + col3 + file0 + + + line59 + col15 + file0 + + + + depth0 + extended_message + Returning from 'setToNull' + message + Returning from 'setToNull' + + + kindcontrol + edges + + +start + + + line60 + col3 + file0 + + + line60 + col3 + file0 + + +end + + + line60 + col8 + file0 + + + line60 + col8 + file0 + + + + + + + kindevent + location + + line60 + col8 + file0 + + ranges + + + + line60 + col4 + file0 + + + line60 + col6 + file0 + + + + depth0 + extended_message + Dereference of null pointer (loaded from variable 'ptr') + message + Dereference of null pointer (loaded from variable 'ptr') + + + macro_expansions + + + location + + line59 + col3 + file0 + + nameTO_NULL + expansionsetToNull(x) + + + descriptionDereference of null pointer (loaded from variable 'ptr') + categoryLogic error + typeDereference of null pointer
r346096 - Revert '[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion'
Author: szelethus
Date: Sun Nov 4 06:18:37 2018
New Revision: 346096
URL: http://llvm.org/viewvc/llvm-project?rev=346096&view=rev
Log:
Revert '[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and
primitive expansion'
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=346096&r1=346095&r2=346096&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Sun Nov 4 06:18:37
2018
@@ -16,7 +16,6 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/Lex/Preprocessor.h"
-#include "clang/Lex/TokenConcatenation.h"
#include "clang/Rewrite/Core/HTMLRewrite.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
@@ -401,6 +400,12 @@ void PlistPrinter::ReportNote(raw_ostrea
// Static function definitions.
//===--===//
+static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
+ const Preprocessor &PP) {
+ // TODO: Implement macro expansion.
+ return { "", "" };
+}
+
/// Print coverage information to output stream {@code o}.
/// May modify the used list of files {@code Fids} by inserting new ones.
static void printCoverage(const PathDiagnostic *D,
@@ -716,196 +721,3 @@ void PlistDiagnostics::FlushDiagnosticsI
// Finish.
o << "\n";
}
-
-//===--===//
-// Declarations of helper functions and data structures for expanding macros.
-//===--===//
-
-namespace {
-
-struct MacroNameAndInfo {
- std::string Name;
- const MacroInfo *MI = nullptr;
-
- MacroNameAndInfo(std::string N, const MacroInfo *MI)
-: Name(std::move(N)), MI(MI) {}
-};
-
-/// Helper class for printing tokens.
-class TokenPrinter {
- llvm::raw_ostream &OS;
- const Preprocessor &PP;
-
- Token PrevTok, PrevPrevTok;
- TokenConcatenation ConcatInfo;
-
-public:
- TokenPrinter(llvm::raw_ostream &OS, const Preprocessor &PP)
-: OS(OS), PP(PP), ConcatInfo(PP) {
-PrevTok.setKind(tok::unknown);
-PrevPrevTok.setKind(tok::unknown);
- }
-
- void printToken(const Token &Tok);
-};
-
-} // end of anonymous namespace
-
-static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
- SourceLocation MacroLoc,
- const Preprocessor &PP);
-
-/// Retrieves the name of the macro and its MacroInfo.
-static MacroNameAndInfo getMacroNameAndInfo(SourceLocation ExpanLoc,
-const Preprocessor &PP);
-
-/// Retrieves the ')' token that matches '(' \p It points to.
-static MacroInfo::tokens_iterator getMatchingRParen(
-MacroInfo::tokens_iterator It,
-MacroInfo::tokens_iterator End);
-
-/// Retrieves the macro info for \p II refers to at \p Loc. This is important
-/// because macros can be redefined or undefined.
-static const MacroInfo *getMacroInfoForLocation(const Preprocessor &PP,
-const SourceManager &SM,
-const IdentifierInfo *II,
-SourceLocation Loc);
-
-//===--===//
-// Definitions of helper functions and methods for expanding macros.
-//===--===//
-
-static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
- const Preprocessor &PP) {
-
- llvm::SmallString<200> ExpansionBuf;
- llvm::raw_svector_ostream OS(ExpansionBuf);
- TokenPrinter Printer(OS, PP);
- return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP), OS.str() };
-}
-
-static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
- SourceLocation MacroLoc,
- const Preprocessor &PP) {
-
- const SourceManager &SM = PP.getSourceManager();
-
- MacroNameAndInfo Info = getMacroNameAndInfo(SM.getExpansionLoc(MacroLoc),
PP);
- const MacroInfo *MI = Info.MI;
-
- // Iterate over the macro's tokens and stringify them.
- for (auto It = MI->tokens_begin(), E = MI->tokens_end(); It != E; ++It) {
-Token T = *It;
-
-// If this token is not an identif
[PATCH] D52794: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
Szelethus reopened this revision. Szelethus added a comment. This revision is now accepted and ready to land. Here's to losing a couple more handfuls of hair, tests break on many platforms, so I reverted it. Repository: rL LLVM https://reviews.llvm.org/D52794 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
lebedev.ri updated this revision to Diff 172520.
lebedev.ri marked 6 inline comments as done.
lebedev.ri added a comment.
Better diag for VLA arrays.
https://reviews.llvm.org/D53771
Files:
clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tidy/hicpp/HICPPTidyModule.cpp
clang-tidy/modernize/AvoidCArraysCheck.cpp
clang-tidy/modernize/AvoidCArraysCheck.h
clang-tidy/modernize/CMakeLists.txt
clang-tidy/modernize/ModernizeTidyModule.cpp
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-avoid-c-arrays.rst
docs/clang-tidy/checks/hicpp-avoid-c-arrays.rst
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
test/clang-tidy/modernize-avoid-c-arrays.cpp
Index: test/clang-tidy/modernize-avoid-c-arrays.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-avoid-c-arrays.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s modernize-avoid-c-arrays %t
+
+int a[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int b[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+void foo() {
+ int c[b[0]];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C VLA arrays, use std::vector<> instead
+}
+
+template
+class array {
+ T d[Size];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+array d;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: do not declare C-style arrays, use std::array<> instead
+
+using k = int[4];
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: do not declare C-style arrays, use std::array<> instead
+
+array dk;
+
+template
+class unique_ptr {
+ T *d;
+
+ int e[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+
+unique_ptr d2;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+using k2 = int[];
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: do not declare C-style arrays, use std::array<> instead
+
+unique_ptr dk2;
+
+// Some header
+extern "C" {
+
+int f[] = {1, 2};
+
+int j[1];
+
+inline void bar() {
+ {
+int j[j[0]];
+ }
+}
+
+extern "C++" {
+int f3[] = {1, 2};
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+int j3[1];
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+
+struct Foo {
+ int f3[3] = {1, 2};
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+
+ int j3[1];
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare C-style arrays, use std::array<> instead
+};
+}
+
+struct Bar {
+
+ int f[3] = {1, 2};
+
+ int j[1];
+};
+}
Index: docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-avoid-c-arrays.rst
@@ -0,0 +1,56 @@
+.. title:: clang-tidy - modernize-avoid-c-arrays
+
+modernize-avoid-c-arrays
+
+
+`cppcoreguidelines-avoid-c-arrays` redirects here as an alias for this check.
+
+`hicpp-avoid-c-arrays` redirects here as an alias for this check.
+
+Finds C-style array types and recommend to use ``std::array<>`` /
+``std::vector<>``. All types of C arrays are diagnosed.
+
+However, fix-it are potentially dangerous in header files and are therefore not
+emitted right now.
+
+.. code:: c++
+
+ int a[] = {1, 2}; // warning: do not declare C-style arrays, use std::array<> instead
+
+ int b[1]; // warning: do not declare C-style arrays, use std::array<> instead
+
+ void foo() {
+int c[b[0]]; // warning: do not declare C VLA arrays, use std::vector<> instead
+ }
+
+ template
+ class array {
+T d[Size]; // warning: do not declare C-style arrays, use std::array<> instead
+
+int e[1]; // warning: do not declare C-style arrays, use std::array<> instead
+ };
+
+ array d; // warning: do not declare C-style arrays, use std::array<> instead
+
+ using k = int[4]; // warning: do not declare C-style arrays, use std::array<> instead
+
+
+However, the ``extern "C"`` code is ignored, since it is common to share
+such headers between C code, and C++ code.
+
+.. code:: c++
+
+ // Some header
+ extern "C" {
+
+ int f[] = {1, 2}; // not diagnosed
+
+ int j[1]; // not diagnosed
+
+ inline void bar() {
+{
+ int j[j[0]]; // not diagnosed
+}
+ }
+
+ }
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -87,6 +87,7 @@
cert-msc51-cpp
cert-
[PATCH] D53771: [clang-tidy] Avoid C arrays check
lebedev.ri added inline comments. Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84 "cppcoreguidelines-c-copy-assignment-signature"); +CheckFactories.registerCheck( +"cppcoreguidelines-avoid-c-arrays"); lebedev.ri wrote: > JonasToth wrote: > > lebedev.ri wrote: > > > JonasToth wrote: > > > > please conserve the alphabetical order here > > > Sorted all the `CheckFactories.registerCheck<>();` lines. > > the `avoid-*` checks seem to be displaced now. > How it should be sorted? By the new check name? Or the internal class name? > If it is not the latter, then sorting is really problematic as it can't be > automatized. Maybe now is better? https://reviews.llvm.org/D53771 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53771: [clang-tidy] Avoid C arrays check
JonasToth added inline comments. Comment at: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:84 "cppcoreguidelines-c-copy-assignment-signature"); +CheckFactories.registerCheck( +"cppcoreguidelines-avoid-c-arrays"); lebedev.ri wrote: > lebedev.ri wrote: > > JonasToth wrote: > > > lebedev.ri wrote: > > > > JonasToth wrote: > > > > > please conserve the alphabetical order here > > > > Sorted all the `CheckFactories.registerCheck<>();` lines. > > > the `avoid-*` checks seem to be displaced now. > > How it should be sorted? By the new check name? Or the internal class name? > > If it is not the latter, then sorting is really problematic as it can't be > > automatized. > Maybe now is better? yes, thats the correct position :) https://reviews.llvm.org/D53771 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); Szelethus wrote: > Szelethus wrote: > > ZaMaZaN4iK wrote: > > > lebedev.ri wrote: > > > > ZaMaZaN4iK wrote: > > > > > lebedev.ri wrote: > > > > > > ZaMaZaN4iK wrote: > > > > > > > lebedev.ri wrote: > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > `const auto *` > > > > > > > > > Why do we need this change here? If I understand correctly, > > > > > > > > > with `const auto*` we also need change initializer to > > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > > But I don't understand why we need this. > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an > > > > > > > > actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > > (sidenote: would be great to have a clang-tidy check for this.) > > > > > > > `ValueToCastOptional` is `llvm::Optional` > > > > > > See, all my guesses were wrong. That is why it should not be `auto` > > > > > > at all. > > > > > I don't agree with you for this case. Honestly it's like a yet > > > > > another holywar question. If we are talking only about this case - > > > > > here you can see `getAs` part of the > > > > > expression. this means clearly (at least for me) that we get > > > > > something like `DefinedOrUnknownSVal`. What we get? I just press > > > > > hotkey in my favourite IDE/text editor and see that `getAs` returns > > > > > `llvm::Optional`. From my point of view it's > > > > > clear enough here. > > > > > > > > > > If we are talking more generally about question "When should we use > > > > > `auto` at all? " - we can talk, but not here, I think :) > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > > comes to mind. > > > > > What we get? I just press hotkey in my favourite IDE/text editor and > > > > > see that getAs returns llvm::Optional > > > > Which hotkey do i need to press to see this here, in the phabricator? > > > > > > > > This really shouldn't be `auto`, if you have to explain that in the > > > > variable's name, justify it in review comments. > > > Ok, didn't know about such LLVM coding standard. Of course, with this > > > information I will fix using `auto` here. Thank you. > > Actually, I disagree. In the Static Analyzer we use `auto` if the return > > type is in the name of the expression, and `getAs` may fail, so it returns > > an `Optional`. In the case where a nullptr may be returned to signal > > failure, `auto *` is used, so I believe that `auto` is appropriate here. > But don't change it back now, it doesn't matter a whole lot :D > Actually, I disagree. In the Static Analyzer we use auto if the return type > is in the name of the expression, and getAs may fail, so it returns an > Optional. The static analyzer should be following the same coding standard as the rest of the project. This is new code, so it's not matching inappropriate styles from older code, so there's really no reason to not match the coding standard. > In the case where a nullptr may be returned to signal failure, auto * is > used, so I believe that auto is appropriate here. I disagree that `auto` is appropriate here. The amount of confusion about the type demonstrates why. https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); aaron.ballman wrote: > Szelethus wrote: > > Szelethus wrote: > > > ZaMaZaN4iK wrote: > > > > lebedev.ri wrote: > > > > > ZaMaZaN4iK wrote: > > > > > > lebedev.ri wrote: > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > lebedev.ri wrote: > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > > `const auto *` > > > > > > > > > > Why do we need this change here? If I understand correctly, > > > > > > > > > > with `const auto*` we also need change initializer to > > > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > > > But I don't understand why we need this. > > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an > > > > > > > > > actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > > > (sidenote: would be great to have a clang-tidy check for > > > > > > > > > this.) > > > > > > > > `ValueToCastOptional` is `llvm::Optional` > > > > > > > See, all my guesses were wrong. That is why it should not be > > > > > > > `auto` at all. > > > > > > I don't agree with you for this case. Honestly it's like a yet > > > > > > another holywar question. If we are talking only about this case - > > > > > > here you can see `getAs` part of the > > > > > > expression. this means clearly (at least for me) that we get > > > > > > something like `DefinedOrUnknownSVal`. What we get? I just press > > > > > > hotkey in my favourite IDE/text editor and see that `getAs` returns > > > > > > `llvm::Optional`. From my point of view it's > > > > > > clear enough here. > > > > > > > > > > > > If we are talking more generally about question "When should we use > > > > > > `auto` at all? " - we can talk, but not here, I think :) > > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > > > comes to mind. > > > > > > What we get? I just press hotkey in my favourite IDE/text editor > > > > > > and see that getAs returns llvm::Optional > > > > > Which hotkey do i need to press to see this here, in the phabricator? > > > > > > > > > > This really shouldn't be `auto`, if you have to explain that in the > > > > > variable's name, justify it in review comments. > > > > Ok, didn't know about such LLVM coding standard. Of course, with this > > > > information I will fix using `auto` here. Thank you. > > > Actually, I disagree. In the Static Analyzer we use `auto` if the return > > > type is in the name of the expression, and `getAs` may fail, so it > > > returns an `Optional`. In the case where a nullptr may be returned to > > > signal failure, `auto *` is used, so I believe that `auto` is appropriate > > > here. > > But don't change it back now, it doesn't matter a whole lot :D > > Actually, I disagree. In the Static Analyzer we use auto if the return type > > is in the name of the expression, and getAs may fail, so it returns an > > Optional. > > The static analyzer should be following the same coding standard as the rest > of the project. This is new code, so it's not matching inappropriate styles > from older code, so there's really no reason to not match the coding standard. > > > In the case where a nullptr may be returned to signal failure, auto * is > > used, so I believe that auto is appropriate here. > > I disagree that `auto` is appropriate here. The amount of confusion about the > type demonstrates why. I confirm the strong local tradition of preferring `auto` for optional `SVal`s in new code, and i believe it's well-justified from the overall coding guideline's point of view. Even if you guys didn't get it right from the start, the amount of Analyzer-specific experience required to understand that it's an optional is very minimal and people get used to it very quickly when they start actively working on the codebase. On one hand, being a class that combines LLVM custom RTTI with pass-by-value semantics (i.e., it's like `QualType` when it comes to passing it around and like `Type` when it comes to casting), optionals are inevitable for representing `SVal` dynamic cast results. On the other hand, `SVal` is one of the most common classes of objects in the Static Analyzer (like, maybe, `Stmt` in Clang; i think a lot of people who are interested in Static Analyzer more than in the rest of Clang learn about `SVal`s earlier than about `Stmt`s), and in particular `SVal` casts are extremely common (around 3-4 `SVal::getAs()` casts per a path-sensitive checker, not counting `castAs`, ~2x more common than arithmetic operations over `SVal`s, only ~3x less common than all sorts of `dyn_cast` in all checkers, including path-insensitive checkers), so it's something you get used to really quickl
r346103 - Add support of the next Ubuntu (Ubuntu 19.04 - Disco Dingo)
Author: sylvestre
Date: Sun Nov 4 09:41:41 2018
New Revision: 346103
URL: http://llvm.org/viewvc/llvm-project?rev=346103&view=rev
Log:
Add support of the next Ubuntu (Ubuntu 19.04 - Disco Dingo)
Modified:
cfe/trunk/include/clang/Driver/Distro.h
cfe/trunk/lib/Driver/Distro.cpp
Modified: cfe/trunk/include/clang/Driver/Distro.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Distro.h?rev=346103&r1=346102&r2=346103&view=diff
==
--- cfe/trunk/include/clang/Driver/Distro.h (original)
+++ cfe/trunk/include/clang/Driver/Distro.h Sun Nov 4 09:41:41 2018
@@ -62,6 +62,7 @@ public:
UbuntuArtful,
UbuntuBionic,
UbuntuCosmic,
+UbuntuDisco,
UnknownDistro
};
@@ -115,7 +116,7 @@ public:
}
bool IsUbuntu() const {
-return DistroVal >= UbuntuHardy && DistroVal <= UbuntuCosmic;
+return DistroVal >= UbuntuHardy && DistroVal <= UbuntuDisco;
}
bool IsAlpineLinux() const {
Modified: cfe/trunk/lib/Driver/Distro.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Distro.cpp?rev=346103&r1=346102&r2=346103&view=diff
==
--- cfe/trunk/lib/Driver/Distro.cpp (original)
+++ cfe/trunk/lib/Driver/Distro.cpp Sun Nov 4 09:41:41 2018
@@ -51,6 +51,7 @@ static Distro::DistroType DetectDistro(l
.Case("artful", Distro::UbuntuArtful)
.Case("bionic", Distro::UbuntuBionic)
.Case("cosmic", Distro::UbuntuCosmic)
+ .Case("disco", Distro::UbuntuDisco)
.Default(Distro::UnknownDistro);
if (Version != Distro::UnknownDistro)
return Version;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > wuzish wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > wuzish wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > wuzish wrote:
> > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > Considering that this is a local lambda called in one
> > > > > > > > > > > place, are we expecting cases where the canonical type is
> > > > > > > > > > > not something on which we can call getVectorKind()? If
> > > > > > > > > > > not, then we do not need this `if`.
> > > > > > > > > > Well, that's for ExtVectorType. I encounter some testcase
> > > > > > > > > > failure because of that. So I just narrow the condition to
> > > > > > > > > > only handle Type::Vector.
> > > > > > > > > >
> > > > > > > > > > As the following comment said, so I treat it separately.
> > > > > > > > > >
> > > > > > > > > > /// ExtVectorType - Extended vector type. This type is
> > > > > > > > > > created using
> > > > > > > > > > /// __attribute__((ext_vector_type(n)), where "n" is the
> > > > > > > > > > number of elements.
> > > > > > > > > > /// Unlike vector_size, ext_vector_type is only allowed on
> > > > > > > > > > typedef's. This
> > > > > > > > > > /// class enables syntactic extensions, like Vector
> > > > > > > > > > Components for accessing
> > > > > > > > > > /// points (as .xyzw), colors (as .rgba), and textures
> > > > > > > > > > (modeled after OpenGL
> > > > > > > > > > /// Shading Language).
> > > > > > > > > An ExtVectorType is a VectorType, so what sort of failures
> > > > > > > > > are you hitting?
> > > > > > > > Yes. But the TypeClass of ExtVectorType is ExtVector.
> > > > > > > >
> > > > > > > > some test points check the error report for ambiguous call
> > > > > > > > because of too many implicit cast choices from ext_vector_type
> > > > > > > > to vector type. Such as blow.
> > > > > > > >
> > > > > > > >
> > > > > > > > ```
> > > > > > > > typedef char char16 __attribute__ ((__vector_size__ (16)));
> > > > > > > > typedef long long longlong16 __attribute__ ((__vector_size__
> > > > > > > > (16)));
> > > > > > > > typedef char char16_e __attribute__ ((__ext_vector_type__
> > > > > > > > (16)));
> > > > > > > > typedef long long longlong16_e __attribute__
> > > > > > > > ((__ext_vector_type__ (2)));
> > > > > > > >
> > > > > > > >
> > > > > > > > void f1_test(char16 c16, longlong16 ll16, char16_e c16e,
> > > > > > > > longlong16_e ll16e) {
> > > > > > > > int &ir1 = f1(c16);
> > > > > > > > float &fr1 = f1(ll16);
> > > > > > > > f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
> > > > > > > > f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > If we are gonna widen the condition, we can make a follow-up
> > > > > > > > patch. Or we need include this condition and do this together
> > > > > > > > in this patch?
> > > > > > > The widening that has occurred is in allowing the scope of the
> > > > > > > change to encompass cases where AltiVec vectors are not
> > > > > > > sufficiently involved. The change in behaviour makes sense, and
> > > > > > > perhaps the community may want to pursue it; however, the mandate
> > > > > > > to make that level of change has not been given.
> > > > > > >
> > > > > > > I do not believe that requiring that the TypeClass be VectorType
> > > > > > > is the correct narrowing of the scope. Instead, we may want to
> > > > > > > consider requiring that for each `SCS` in { `SCS1`, `SCS2` },
> > > > > > > there is one AltiVec type and one generic vector type in {
> > > > > > > `SCS.getFromType()`, `SCS.getToType(2)` }.
> > > > > > >
> > > > > > The key point is that ExtVector is a kind of typeclass, **and** its
> > > > > > vector kind is generic vector type.
> > > > > >
> > > > > > So you think we should encompass ext_vector cases as a part of the
> > > > > > scope of this patch? And modify the above cases' expected behavior
> > > > > > (remove the **expected-error**)?
> > > > > I gave a concrete suggested narrowing of the scope that does not
> > > > > exclude ExtVectorType or other derived types of VectorType. It also
> > > > > does not change the behaviour of the `f1_test` case above. We could
> > > > > pursue additional discussion over that case (separable from the
> > > > > current patch) on the mailing list.
> > > > >
> > > > > I believe my suggestion does do something about this case:
> > > > > ```
> > > > > typedef unsigned int GccType __attribute__((__ext_vector_type__(16)));
> > > > > typedef __
[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.
mattd added a comment. In https://reviews.llvm.org/D54048#1286654, @rjmccall wrote: > That looks a lot better, thanks. Did you check whether any other tests > needed adjustment? That's not uncommon from this kind of desugaring change. > > If not, LGTM. Thanks again. I ran the typical 'check-all' in my working tree which included clang, clang-extras, compiler-rt, lld. Everything passed as expected. https://reviews.llvm.org/D54048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); NoQ wrote: > aaron.ballman wrote: > > Szelethus wrote: > > > Szelethus wrote: > > > > ZaMaZaN4iK wrote: > > > > > lebedev.ri wrote: > > > > > > ZaMaZaN4iK wrote: > > > > > > > lebedev.ri wrote: > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > lebedev.ri wrote: > > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > > > `const auto *` > > > > > > > > > > > Why do we need this change here? If I understand > > > > > > > > > > > correctly, with `const auto*` we also need change > > > > > > > > > > > initializer to > > > > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > > > > But I don't understand why we need this. > > > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just an > > > > > > > > > > actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > > > > (sidenote: would be great to have a clang-tidy check for > > > > > > > > > > this.) > > > > > > > > > `ValueToCastOptional` is > > > > > > > > > `llvm::Optional` > > > > > > > > See, all my guesses were wrong. That is why it should not be > > > > > > > > `auto` at all. > > > > > > > I don't agree with you for this case. Honestly it's like a yet > > > > > > > another holywar question. If we are talking only about this case > > > > > > > - here you can see `getAs` part of the > > > > > > > expression. this means clearly (at least for me) that we get > > > > > > > something like `DefinedOrUnknownSVal`. What we get? I just press > > > > > > > hotkey in my favourite IDE/text editor and see that `getAs` > > > > > > > returns `llvm::Optional`. From my point of > > > > > > > view it's clear enough here. > > > > > > > > > > > > > > If we are talking more generally about question "When should we > > > > > > > use `auto` at all? " - we can talk, but not here, I think :) > > > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > > > > comes to mind. > > > > > > > What we get? I just press hotkey in my favourite IDE/text editor > > > > > > > and see that getAs returns llvm::Optional > > > > > > Which hotkey do i need to press to see this here, in the > > > > > > phabricator? > > > > > > > > > > > > This really shouldn't be `auto`, if you have to explain that in the > > > > > > variable's name, justify it in review comments. > > > > > Ok, didn't know about such LLVM coding standard. Of course, with this > > > > > information I will fix using `auto` here. Thank you. > > > > Actually, I disagree. In the Static Analyzer we use `auto` if the > > > > return type is in the name of the expression, and `getAs` may fail, so > > > > it returns an `Optional`. In the case where a nullptr may be returned > > > > to signal failure, `auto *` is used, so I believe that `auto` is > > > > appropriate here. > > > But don't change it back now, it doesn't matter a whole lot :D > > > Actually, I disagree. In the Static Analyzer we use auto if the return > > > type is in the name of the expression, and getAs may fail, so it returns > > > an Optional. > > > > The static analyzer should be following the same coding standard as the > > rest of the project. This is new code, so it's not matching inappropriate > > styles from older code, so there's really no reason to not match the coding > > standard. > > > > > In the case where a nullptr may be returned to signal failure, auto * is > > > used, so I believe that auto is appropriate here. > > > > I disagree that `auto` is appropriate here. The amount of confusion about > > the type demonstrates why. > I confirm the strong local tradition of preferring `auto` for optional > `SVal`s in new code, and i believe it's well-justified from the overall > coding guideline's point of view. Even if you guys didn't get it right from > the start, the amount of Analyzer-specific experience required to understand > that it's an optional is very minimal and people get used to it very quickly > when they start actively working on the codebase. > > On one hand, being a class that combines LLVM custom RTTI with pass-by-value > semantics (i.e., it's like `QualType` when it comes to passing it around and > like `Type` when it comes to casting), optionals are inevitable for > representing `SVal` dynamic cast results. > > On the other hand, `SVal` is one of the most common classes of objects in the > Static Analyzer (like, maybe, `Stmt` in Clang; i think a lot of people who > are interested in Static Analyzer more than in the rest of Clang learn about > `SVal`s earlier than about `Stmt`s), and in particular `SVal` casts are > extremely common (around 3-4 `SVal::getAs()` casts per a path-sensitive > c
r346107 - [Driver] Use -Bstatic/dynamic for libc++ on Fuchsia
Author: phosek
Date: Sun Nov 4 14:38:47 2018
New Revision: 346107
URL: http://llvm.org/viewvc/llvm-project?rev=346107&view=rev
Log:
[Driver] Use -Bstatic/dynamic for libc++ on Fuchsia
-static relies on lld's behavior, but -Bstatic/dynamic is supported
across all linkers.
Differential Revision: https://reviews.llvm.org/D54082
Modified:
cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
cfe/trunk/test/Driver/fuchsia.cpp
Modified: cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp?rev=346107&r1=346106&r2=346107&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp Sun Nov 4 14:38:47 2018
@@ -125,8 +125,10 @@ void fuchsia::Linker::ConstructJob(Compi
CmdArgs.push_back("--push-state");
CmdArgs.push_back("--as-needed");
if (OnlyLibstdcxxStatic)
- CmdArgs.push_back("-static");
+ CmdArgs.push_back("-Bstatic");
ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
+if (OnlyLibstdcxxStatic)
+ CmdArgs.push_back("-Bdynamic");
CmdArgs.push_back("-lm");
CmdArgs.push_back("--pop-state");
}
Modified: cfe/trunk/test/Driver/fuchsia.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/fuchsia.cpp?rev=346107&r1=346106&r2=346107&view=diff
==
--- cfe/trunk/test/Driver/fuchsia.cpp (original)
+++ cfe/trunk/test/Driver/fuchsia.cpp Sun Nov 4 14:38:47 2018
@@ -37,8 +37,9 @@
// RUN: | FileCheck %s -check-prefix=CHECK-STATIC
// CHECK-STATIC: "--push-state"
// CHECK-STATIC: "--as-needed"
-// CHECK-STATIC: "-static"
+// CHECK-STATIC: "-Bstatic"
// CHECK-STATIC: "-lc++"
+// CHECK-STATIC: "-Bdynamic"
// CHECK-STATIC: "-lm"
// CHECK-STATIC: "--pop-state"
// CHECK-STATIC: "-lc"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54082: [Driver] Use -Bstatic/dynamic for libc++ on Fuchsia
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346107: [Driver] Use -Bstatic/dynamic for libc++ on Fuchsia
(authored by phosek, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54082?vs=172513&id=172530#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54082
Files:
cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
cfe/trunk/test/Driver/fuchsia.cpp
Index: cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
@@ -125,8 +125,10 @@
CmdArgs.push_back("--push-state");
CmdArgs.push_back("--as-needed");
if (OnlyLibstdcxxStatic)
- CmdArgs.push_back("-static");
+ CmdArgs.push_back("-Bstatic");
ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
+if (OnlyLibstdcxxStatic)
+ CmdArgs.push_back("-Bdynamic");
CmdArgs.push_back("-lm");
CmdArgs.push_back("--pop-state");
}
Index: cfe/trunk/test/Driver/fuchsia.cpp
===
--- cfe/trunk/test/Driver/fuchsia.cpp
+++ cfe/trunk/test/Driver/fuchsia.cpp
@@ -37,8 +37,9 @@
// RUN: | FileCheck %s -check-prefix=CHECK-STATIC
// CHECK-STATIC: "--push-state"
// CHECK-STATIC: "--as-needed"
-// CHECK-STATIC: "-static"
+// CHECK-STATIC: "-Bstatic"
// CHECK-STATIC: "-lc++"
+// CHECK-STATIC: "-Bdynamic"
// CHECK-STATIC: "-lm"
// CHECK-STATIC: "--pop-state"
// CHECK-STATIC: "-lc"
Index: cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Fuchsia.cpp
@@ -125,8 +125,10 @@
CmdArgs.push_back("--push-state");
CmdArgs.push_back("--as-needed");
if (OnlyLibstdcxxStatic)
- CmdArgs.push_back("-static");
+ CmdArgs.push_back("-Bstatic");
ToolChain.AddCXXStdlibLibArgs(Args, CmdArgs);
+if (OnlyLibstdcxxStatic)
+ CmdArgs.push_back("-Bdynamic");
CmdArgs.push_back("-lm");
CmdArgs.push_back("--pop-state");
}
Index: cfe/trunk/test/Driver/fuchsia.cpp
===
--- cfe/trunk/test/Driver/fuchsia.cpp
+++ cfe/trunk/test/Driver/fuchsia.cpp
@@ -37,8 +37,9 @@
// RUN: | FileCheck %s -check-prefix=CHECK-STATIC
// CHECK-STATIC: "--push-state"
// CHECK-STATIC: "--as-needed"
-// CHECK-STATIC: "-static"
+// CHECK-STATIC: "-Bstatic"
// CHECK-STATIC: "-lc++"
+// CHECK-STATIC: "-Bdynamic"
// CHECK-STATIC: "-lm"
// CHECK-STATIC: "--pop-state"
// CHECK-STATIC: "-lc"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53995: [analyzer] Drastically simplify the tblgen files used for checkers
Szelethus updated this revision to Diff 172533.
Szelethus added a comment.
- Came to the shocking realization that `Hidden` is also an unused property, so
I removed that too.
- Added comments to `CheckerBase.td`.
- Added missing ` // end "packagename"` comments
Now that `CheckerBase.td` isn't that cryptic, maybe it isn't worth pursuing the
.def file conversion -- it probably would take a lot of effort for little
value. My main issue was the maintenance of these tblgen files, but not that
they have been simplified, I think it's fine to leave as is.
https://reviews.llvm.org/D53995
Files:
include/clang/Driver/CC1Options.td
include/clang/StaticAnalyzer/Checkers/CheckerBase.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/ClangCheckers.cpp
lib/StaticAnalyzer/Checkers/ClangSACheckers.h
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
utils/TableGen/ClangSACheckersEmitter.cpp
Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -11,12 +11,13 @@
//
//===--===//
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/TableGen/Error.h"
#include "llvm/TableGen/Record.h"
#include "llvm/TableGen/TableGenBackend.h"
#include
#include
+
using namespace llvm;
//===--===//
@@ -35,32 +36,32 @@
return false;
}
-static bool isCheckerNamed(const Record *R) {
- return !R->getValueAsString("CheckerName").empty();
-}
-
static std::string getPackageFullName(const Record *R);
static std::string getParentPackageFullName(const Record *R) {
std::string name;
- if (DefInit *DI = dyn_cast(R->getValueInit("ParentPackage")))
+ if (DefInit *DI = dyn_cast(R->getValueInit("ParentPackage"))) {
name = getPackageFullName(DI->getDef());
- return name;
+R->dump();
+ }
+ return name;
}
static std::string getPackageFullName(const Record *R) {
std::string name = getParentPackageFullName(R);
- if (!name.empty()) name += ".";
+ if (!name.empty())
+name += ".";
+ assert(!R->getValueAsString("PackageName").empty());
name += R->getValueAsString("PackageName");
return name;
}
static std::string getCheckerFullName(const Record *R) {
std::string name = getParentPackageFullName(R);
- if (isCheckerNamed(R)) {
-if (!name.empty()) name += ".";
-name += R->getValueAsString("CheckerName");
- }
+ if (!name.empty())
+name += ".";
+ assert(!R->getValueAsString("CheckerName").empty());
+ name += R->getValueAsString("CheckerName");
return name;
}
@@ -70,129 +71,12 @@
return std::string();
}
-namespace {
-struct GroupInfo {
- llvm::DenseSet Checkers;
- llvm::DenseSet SubGroups;
- bool Hidden;
- unsigned Index;
-
- GroupInfo() : Hidden(false) { }
-};
-}
-
-static void addPackageToCheckerGroup(const Record *package, const Record *group,
- llvm::DenseMap &recordGroupMap) {
- llvm::DenseSet &checkers = recordGroupMap[package]->Checkers;
- for (llvm::DenseSet::iterator
- I = checkers.begin(), E = checkers.end(); I != E; ++I)
-recordGroupMap[group]->Checkers.insert(*I);
-
- llvm::DenseSet &subGroups = recordGroupMap[package]->SubGroups;
- for (llvm::DenseSet::iterator
- I = subGroups.begin(), E = subGroups.end(); I != E; ++I)
-addPackageToCheckerGroup(*I, group, recordGroupMap);
-}
-
namespace clang {
void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
std::vector checkers = Records.getAllDerivedDefinitions("Checker");
- llvm::DenseMap checkerRecIndexMap;
- for (unsigned i = 0, e = checkers.size(); i != e; ++i)
-checkerRecIndexMap[checkers[i]] = i;
-
- // Invert the mapping of checkers to package/group into a one to many
- // mapping of packages/groups to checkers.
- std::map groupInfoByName;
- llvm::DenseMap recordGroupMap;
-
std::vector packages = Records.getAllDerivedDefinitions("Package");
- for (unsigned i = 0, e = packages.size(); i != e; ++i) {
-Record *R = packages[i];
-std::string fullName = getPackageFullName(R);
-if (!fullName.empty()) {
- GroupInfo &info = groupInfoByName[fullName];
- info.Hidden = isHidden(*R);
- recordGroupMap[R] = &info;
-}
- }
-
- std::vector
- checkerGroups = Records.getAllDerivedDefinitions("CheckerGroup");
- for (unsigned i = 0, e = checkerGroups.size(); i != e; ++i) {
-Record *R = checkerGroups[i];
-std::string name = R->getValueAsString("GroupName");
-if (!name.empty()) {
- GroupInfo &info = groupInfoByName[name];
- recordGroupMap[R] = &info;
-}
- }
- for (unsigned i = 0, e = checkers.size(); i != e; ++i) {
-Record *R = checkers[i];
-Record *package = nullptr;
-if (DefIni
[PATCH] D53995: [analyzer] Drastically simplify the tblgen files used for checkers
Szelethus updated this revision to Diff 172534.
https://reviews.llvm.org/D53995
Files:
include/clang/Driver/CC1Options.td
include/clang/StaticAnalyzer/Checkers/CheckerBase.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/ClangCheckers.cpp
lib/StaticAnalyzer/Checkers/ClangSACheckers.h
lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
utils/TableGen/ClangSACheckersEmitter.cpp
Index: utils/TableGen/ClangSACheckersEmitter.cpp
===
--- utils/TableGen/ClangSACheckersEmitter.cpp
+++ utils/TableGen/ClangSACheckersEmitter.cpp
@@ -11,12 +11,13 @@
//
//===--===//
-#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringMap.h"
#include "llvm/TableGen/Error.h"
#include "llvm/TableGen/Record.h"
#include "llvm/TableGen/TableGenBackend.h"
#include
#include
+
using namespace llvm;
//===--===//
@@ -35,32 +36,30 @@
return false;
}
-static bool isCheckerNamed(const Record *R) {
- return !R->getValueAsString("CheckerName").empty();
-}
-
static std::string getPackageFullName(const Record *R);
static std::string getParentPackageFullName(const Record *R) {
std::string name;
if (DefInit *DI = dyn_cast(R->getValueInit("ParentPackage")))
name = getPackageFullName(DI->getDef());
- return name;
+ return name;
}
static std::string getPackageFullName(const Record *R) {
std::string name = getParentPackageFullName(R);
- if (!name.empty()) name += ".";
+ if (!name.empty())
+name += ".";
+ assert(!R->getValueAsString("PackageName").empty());
name += R->getValueAsString("PackageName");
return name;
}
static std::string getCheckerFullName(const Record *R) {
std::string name = getParentPackageFullName(R);
- if (isCheckerNamed(R)) {
-if (!name.empty()) name += ".";
-name += R->getValueAsString("CheckerName");
- }
+ if (!name.empty())
+name += ".";
+ assert(!R->getValueAsString("CheckerName").empty());
+ name += R->getValueAsString("CheckerName");
return name;
}
@@ -70,129 +69,12 @@
return std::string();
}
-namespace {
-struct GroupInfo {
- llvm::DenseSet Checkers;
- llvm::DenseSet SubGroups;
- bool Hidden;
- unsigned Index;
-
- GroupInfo() : Hidden(false) { }
-};
-}
-
-static void addPackageToCheckerGroup(const Record *package, const Record *group,
- llvm::DenseMap &recordGroupMap) {
- llvm::DenseSet &checkers = recordGroupMap[package]->Checkers;
- for (llvm::DenseSet::iterator
- I = checkers.begin(), E = checkers.end(); I != E; ++I)
-recordGroupMap[group]->Checkers.insert(*I);
-
- llvm::DenseSet &subGroups = recordGroupMap[package]->SubGroups;
- for (llvm::DenseSet::iterator
- I = subGroups.begin(), E = subGroups.end(); I != E; ++I)
-addPackageToCheckerGroup(*I, group, recordGroupMap);
-}
-
namespace clang {
void EmitClangSACheckers(RecordKeeper &Records, raw_ostream &OS) {
std::vector checkers = Records.getAllDerivedDefinitions("Checker");
- llvm::DenseMap checkerRecIndexMap;
- for (unsigned i = 0, e = checkers.size(); i != e; ++i)
-checkerRecIndexMap[checkers[i]] = i;
-
- // Invert the mapping of checkers to package/group into a one to many
- // mapping of packages/groups to checkers.
- std::map groupInfoByName;
- llvm::DenseMap recordGroupMap;
-
std::vector packages = Records.getAllDerivedDefinitions("Package");
- for (unsigned i = 0, e = packages.size(); i != e; ++i) {
-Record *R = packages[i];
-std::string fullName = getPackageFullName(R);
-if (!fullName.empty()) {
- GroupInfo &info = groupInfoByName[fullName];
- info.Hidden = isHidden(*R);
- recordGroupMap[R] = &info;
-}
- }
-
- std::vector
- checkerGroups = Records.getAllDerivedDefinitions("CheckerGroup");
- for (unsigned i = 0, e = checkerGroups.size(); i != e; ++i) {
-Record *R = checkerGroups[i];
-std::string name = R->getValueAsString("GroupName");
-if (!name.empty()) {
- GroupInfo &info = groupInfoByName[name];
- recordGroupMap[R] = &info;
-}
- }
-
- for (unsigned i = 0, e = checkers.size(); i != e; ++i) {
-Record *R = checkers[i];
-Record *package = nullptr;
-if (DefInit *
- DI = dyn_cast(R->getValueInit("ParentPackage")))
- package = DI->getDef();
-if (!isCheckerNamed(R) && !package)
- PrintFatalError(R->getLoc(), "Checker '" + R->getName() +
- "' is neither named, nor in a package!");
-
-if (isCheckerNamed(R)) {
- // Create a pseudo-group to hold this checker.
- std::string fullName = getCheckerFullName(R);
- GroupInfo &info = groupInfoByName[fullName];
- info.Hidden = R->getValueAsBit("Hidden");
- recordGroupMap[R] = &info;
- info.Checkers.insert(R);
-} else {
- recordGr
[PATCH] D53982: Output "rule" information in SARIF
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Checkers.inc" +#undef CHECKER +#undef GET_CHECKERS Hmmm, this won't handle checkers loaded from plugins. https://reviews.llvm.org/D53982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53982: Output "rule" information in SARIF
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Checkers.inc" +#undef CHECKER +#undef GET_CHECKERS Szelethus wrote: > Hmmm, this won't handle checkers loaded from plugins. I don't immediately know the solution is to this, but when you invoke clang with `-cc1 -analyzer-checker-help`, plugin checkers are also displayed, and [[ https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp#L150 | this is line that magically does it ]]. Maybe store the plugins in `AnalyzerOptions`, and move `ClangCheckerRegistry` to `include/clang/StaticAnalyzer/Frontend`? https://reviews.llvm.org/D53982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346111 - Reland '[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion'
Author: szelethus
Date: Sun Nov 4 18:14:36 2018
New Revision: 346111
URL: http://llvm.org/viewvc/llvm-project?rev=346111&view=rev
Log:
Reland '[analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and
primitive expansion'
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
cfe/trunk/test/Analysis/plist-macros-with-expansion.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=346111&r1=346110&r2=346111&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Sun Nov 4 18:14:36
2018
@@ -16,6 +16,7 @@
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/Version.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/TokenConcatenation.h"
#include "clang/Rewrite/Core/HTMLRewrite.h"
#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
@@ -400,12 +401,6 @@ void PlistPrinter::ReportNote(raw_ostrea
// Static function definitions.
//===--===//
-static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
- const Preprocessor &PP) {
- // TODO: Implement macro expansion.
- return { "", "" };
-}
-
/// Print coverage information to output stream {@code o}.
/// May modify the used list of files {@code Fids} by inserting new ones.
static void printCoverage(const PathDiagnostic *D,
@@ -721,3 +716,196 @@ void PlistDiagnostics::FlushDiagnosticsI
// Finish.
o << "\n";
}
+
+//===--===//
+// Declarations of helper functions and data structures for expanding macros.
+//===--===//
+
+namespace {
+
+struct MacroNameAndInfo {
+ std::string Name;
+ const MacroInfo *MI = nullptr;
+
+ MacroNameAndInfo(std::string N, const MacroInfo *MI)
+: Name(std::move(N)), MI(MI) {}
+};
+
+/// Helper class for printing tokens.
+class TokenPrinter {
+ llvm::raw_ostream &OS;
+ const Preprocessor &PP;
+
+ Token PrevTok, PrevPrevTok;
+ TokenConcatenation ConcatInfo;
+
+public:
+ TokenPrinter(llvm::raw_ostream &OS, const Preprocessor &PP)
+: OS(OS), PP(PP), ConcatInfo(PP) {
+PrevTok.setKind(tok::unknown);
+PrevPrevTok.setKind(tok::unknown);
+ }
+
+ void printToken(const Token &Tok);
+};
+
+} // end of anonymous namespace
+
+static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
+ SourceLocation MacroLoc,
+ const Preprocessor &PP);
+
+/// Retrieves the name of the macro and its MacroInfo.
+static MacroNameAndInfo getMacroNameAndInfo(SourceLocation ExpanLoc,
+const Preprocessor &PP);
+
+/// Retrieves the ')' token that matches '(' \p It points to.
+static MacroInfo::tokens_iterator getMatchingRParen(
+MacroInfo::tokens_iterator It,
+MacroInfo::tokens_iterator End);
+
+/// Retrieves the macro info for \p II refers to at \p Loc. This is important
+/// because macros can be redefined or undefined.
+static const MacroInfo *getMacroInfoForLocation(const Preprocessor &PP,
+const SourceManager &SM,
+const IdentifierInfo *II,
+SourceLocation Loc);
+
+//===--===//
+// Definitions of helper functions and methods for expanding macros.
+//===--===//
+
+static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
+ const Preprocessor &PP) {
+
+ llvm::SmallString<200> ExpansionBuf;
+ llvm::raw_svector_ostream OS(ExpansionBuf);
+ TokenPrinter Printer(OS, PP);
+ return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP), OS.str() };
+}
+
+static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
+ SourceLocation MacroLoc,
+ const Preprocessor &PP) {
+
+ const SourceManager &SM = PP.getSourceManager();
+
+ MacroNameAndInfo Info = getMacroNameAndInfo(SM.getExpansionLoc(MacroLoc),
PP);
+ const MacroInfo *MI = Info.MI;
+
+ // Iterate over the macro's tokens and stringify them.
+ for (auto It = MI->tokens_begin(), E = MI->tokens_end(); It != E; ++It) {
+Token T = *It;
+
+// If this token is not an identif
r346112 - Ensure the correct order of evaluation in part 2. of PlistMacroExpansion
Author: szelethus
Date: Sun Nov 4 18:37:29 2018
New Revision: 346112
URL: http://llvm.org/viewvc/llvm-project?rev=346112&view=rev
Log:
Ensure the correct order of evaluation in part 2. of PlistMacroExpansion
Windows buildbots break with the previous commit
'[analyzer][PlistMacroExpansion]
Part 2.: Retrieving the macro name and primitive expansion'. This patch attempts
to solve this issue.
Modified:
cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp?rev=346112&r1=346111&r2=346112&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp Sun Nov 4 18:37:29
2018
@@ -781,7 +781,8 @@ static ExpansionInfo getExpandedMacro(So
llvm::SmallString<200> ExpansionBuf;
llvm::raw_svector_ostream OS(ExpansionBuf);
TokenPrinter Printer(OS, PP);
- return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP), OS.str() };
+ std::string MacroName = getMacroNameAndPrintExpansion(Printer, MacroLoc, PP);
+ return { MacroName, OS.str() };
}
static std::string getMacroNameAndPrintExpansion(TokenPrinter &Printer,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34018: Support __float128 on NetBSD libstdc++ x86/x86_64
krytarowski requested review of this revision. krytarowski added a comment. Herald added a subscriber: llvm-commits. This change happened to be required in downstream usage.. knowing its limits can we merge it as it is? Repository: rL LLVM https://reviews.llvm.org/D34018 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54087: [PowerPC] [Clang] [AltiVec] The second parameter of vec_sr function should be modulo the number of bits in the element
wuzish created this revision.
wuzish added reviewers: hfinkel, nemanjai, kbarton.
Herald added a subscriber: jsji.
The second parameter of vec_sr function is representing shift bits and it
should be modulo the number of bits in the element like what vec_sl does now.
Repository:
rC Clang
https://reviews.llvm.org/D54087
Files:
clang/lib/Headers/altivec.h
clang/test/CodeGen/builtins-ppc-altivec.c
clang/test/CodeGen/builtins-ppc-p8vector.c
Index: clang/test/CodeGen/builtins-ppc-p8vector.c
===
--- clang/test/CodeGen/builtins-ppc-p8vector.c
+++ clang/test/CodeGen/builtins-ppc-p8vector.c
@@ -1066,13 +1066,17 @@
/* vec_sr */
res_vsll = vec_sr(vsll, vull);
-// CHECK: lshr <2 x i64>
-// CHECK-LE: lshr <2 x i64>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <2 x i64> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <2 x i64> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <2 x i64> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <2 x i64> {{[0-9a-zA-Z%.]+}}, [[UREM]]
// CHECK-PPC: error: call to 'vec_sr' is ambiguous
res_vull = vec_sr(vull, vull);
-// CHECK: lshr <2 x i64>
-// CHECK-LE: lshr <2 x i64>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <2 x i64> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <2 x i64> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <2 x i64> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <2 x i64> {{[0-9a-zA-Z%.]+}}, [[UREM]]
// CHECK-PPC: error: call to 'vec_sr' is ambiguous
/* vec_sra */
Index: clang/test/CodeGen/builtins-ppc-altivec.c
===
--- clang/test/CodeGen/builtins-ppc-altivec.c
+++ clang/test/CodeGen/builtins-ppc-altivec.c
@@ -4256,52 +4256,76 @@
/* vec_sr */
res_vsc = vec_sr(vsc, vuc);
-// CHECK: lshr <16 x i8>
-// CHECK-LE: lshr <16 x i8>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vuc = vec_sr(vuc, vuc);
-// CHECK: lshr <16 x i8>
-// CHECK-LE: lshr <16 x i8>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vs = vec_sr(vs, vus);
-// CHECK: lshr <8 x i16>
-// CHECK-LE: lshr <8 x i16>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vus = vec_sr(vus, vus);
-// CHECK: lshr <8 x i16>
-// CHECK-LE: lshr <8 x i16>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vi = vec_sr(vi, vui);
-// CHECK: lshr <4 x i32>
-// CHECK-LE: lshr <4 x i32>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <4 x i32> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <4 x i32> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <4 x i32> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <4 x i32> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vui = vec_sr(vui, vui);
-// CHECK: lshr <4 x i32>
-// CHECK-LE: lshr <4 x i32>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <4 x i32> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <4 x i32> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <4 x i32> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <4 x i32> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vsc = vec_vsrb(vsc, vuc);
-// CHECK: shr <16 x i8>
-// CHECK-LE: shr <16 x i8>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vuc = vec_vsrb(vuc, vuc);
-// CHECK: shr <16 x i8>
-// CHECK-LE: shr <16 x i8>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <16 x i8> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <16 x i8> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vs = vec_vsrh(vs, vus);
-// CHECK: shr <8 x i16>
-// CHECK-LE: shr <8 x i16>
+// CHECK: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
+// CHECK-LE: [[UREM:[0-9a-zA-Z%.]+]] = urem <8 x i16> {{[0-9a-zA-Z%.]+}},
+// CHECK-LE: lshr <8 x i16> {{[0-9a-zA-Z%.]+}}, [[UREM]]
res_vus = vec_vsrh(vus, vus);
-// CHECK: shr <8 x i16>
-// CHE
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked an inline comment as done. wuzish added a comment. Gentle ping. Could anyone else have a more review? https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked an inline comment as done.
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > wuzish wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > wuzish wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > wuzish wrote:
> > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > wuzish wrote:
> > > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > > Considering that this is a local lambda called in one
> > > > > > > > > > > > place, are we expecting cases where the canonical type
> > > > > > > > > > > > is not something on which we can call getVectorKind()?
> > > > > > > > > > > > If not, then we do not need this `if`.
> > > > > > > > > > > Well, that's for ExtVectorType. I encounter some testcase
> > > > > > > > > > > failure because of that. So I just narrow the condition
> > > > > > > > > > > to only handle Type::Vector.
> > > > > > > > > > >
> > > > > > > > > > > As the following comment said, so I treat it separately.
> > > > > > > > > > >
> > > > > > > > > > > /// ExtVectorType - Extended vector type. This type is
> > > > > > > > > > > created using
> > > > > > > > > > > /// __attribute__((ext_vector_type(n)), where "n" is the
> > > > > > > > > > > number of elements.
> > > > > > > > > > > /// Unlike vector_size, ext_vector_type is only allowed
> > > > > > > > > > > on typedef's. This
> > > > > > > > > > > /// class enables syntactic extensions, like Vector
> > > > > > > > > > > Components for accessing
> > > > > > > > > > > /// points (as .xyzw), colors (as .rgba), and textures
> > > > > > > > > > > (modeled after OpenGL
> > > > > > > > > > > /// Shading Language).
> > > > > > > > > > An ExtVectorType is a VectorType, so what sort of failures
> > > > > > > > > > are you hitting?
> > > > > > > > > Yes. But the TypeClass of ExtVectorType is ExtVector.
> > > > > > > > >
> > > > > > > > > some test points check the error report for ambiguous call
> > > > > > > > > because of too many implicit cast choices from
> > > > > > > > > ext_vector_type to vector type. Such as blow.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > ```
> > > > > > > > > typedef char char16 __attribute__ ((__vector_size__ (16)));
> > > > > > > > > typedef long long longlong16 __attribute__ ((__vector_size__
> > > > > > > > > (16)));
> > > > > > > > > typedef char char16_e __attribute__ ((__ext_vector_type__
> > > > > > > > > (16)));
> > > > > > > > > typedef long long longlong16_e __attribute__
> > > > > > > > > ((__ext_vector_type__ (2)));
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > void f1_test(char16 c16, longlong16 ll16, char16_e c16e,
> > > > > > > > > longlong16_e ll16e) {
> > > > > > > > > int &ir1 = f1(c16);
> > > > > > > > > float &fr1 = f1(ll16);
> > > > > > > > > f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
> > > > > > > > > f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > >
> > > > > > > > > If we are gonna widen the condition, we can make a follow-up
> > > > > > > > > patch. Or we need include this condition and do this together
> > > > > > > > > in this patch?
> > > > > > > > The widening that has occurred is in allowing the scope of the
> > > > > > > > change to encompass cases where AltiVec vectors are not
> > > > > > > > sufficiently involved. The change in behaviour makes sense, and
> > > > > > > > perhaps the community may want to pursue it; however, the
> > > > > > > > mandate to make that level of change has not been given.
> > > > > > > >
> > > > > > > > I do not believe that requiring that the TypeClass be
> > > > > > > > VectorType is the correct narrowing of the scope. Instead, we
> > > > > > > > may want to consider requiring that for each `SCS` in { `SCS1`,
> > > > > > > > `SCS2` }, there is one AltiVec type and one generic vector type
> > > > > > > > in { `SCS.getFromType()`, `SCS.getToType(2)` }.
> > > > > > > >
> > > > > > > The key point is that ExtVector is a kind of typeclass, **and**
> > > > > > > its vector kind is generic vector type.
> > > > > > >
> > > > > > > So you think we should encompass ext_vector cases as a part of
> > > > > > > the scope of this patch? And modify the above cases' expected
> > > > > > > behavior (remove the **expected-error**)?
> > > > > > I gave a concrete suggested narrowing of the scope that does not
> > > > > > exclude ExtVectorType or other derived types of VectorType. It also
> > > > > > does not change the behaviour of the `f1_test` case above. We could
> > > > > > pursue additional discussion over that case (separable from the
> > > > > > curr
[PATCH] D52794: [analyzer][PlistMacroExpansion] Part 2.: Retrieving the macro name and primitive expansion
Szelethus closed this revision. Szelethus added a comment. Woohoo, it seems to be fine! ^-^ I thought the evaluation order in braced initializer lists are from left to right, but apparently not. Certainly not on windows. Repository: rL LLVM https://reviews.llvm.org/D52794 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r346113 - [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered
Author: szelethus
Date: Sun Nov 4 19:50:37 2018
New Revision: 346113
URL: http://llvm.org/viewvc/llvm-project?rev=346113&view=rev
Log:
[analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have
to be registered
One of the reasons why AnalyzerOptions is so chaotic is that options can be
retrieved from the command line whenever and wherever. This allowed for some
options to be forgotten for a looong time. Have you ever heard of
"region-store-small-struct-limit"? In order to prevent this in the future, I'm
proposing to restrict AnalyzerOptions' interface so that only checker options
can be retrieved without special getters. I would like to make every option be
accessible only through a getter, but checkers from plugins are a thing, so I'll
have to figure something out for that.
This also forces developers who'd like to add a new option to register it
properly in the .def file.
This is done by
* making the third checker pointer parameter non-optional, and checked by an
assert to be non-null.
* I added new, but private non-checkers option initializers, meant only for
internal use,
* Renamed these methods accordingly (mind the consistent name for once with
getBooleanOption!):
- getOptionAsString -> getCheckerStringOption,
- getOptionAsInteger -> getCheckerIntegerOption
* The 3 functions meant for initializing data members (with the not very
descriptive getBooleanOption, getOptionAsString and getOptionAsUInt names)
were renamed to be overloads of the getAndInitOption function name.
* All options were in some way retrieved via getCheckerOption. I removed it, and
moved the logic to getStringOption and getCheckerStringOption. This did cause
some code duplication, but that's the only way I could do it, now that checker
and non-checker options are separated. Note that the non-checker version
inserts the new option to the ConfigTable with the default value, but the
checker version only attempts to find already existing entries. This is how
it always worked, but this is clunky and I might end reworking that too, so we
can eventually get a ConfigTable that contains the entire configuration of the
analyzer.
Differential Revision: https://reviews.llvm.org/D53483
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
cfe/trunk/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h?rev=346113&r1=346112&r2=346113&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h Sun Nov 4
19:50:37 2018
@@ -137,6 +137,28 @@ enum UserModeKind {
UMK_Deep = 2
};
+/// Stores options for the analyzer from the command line.
+///
+/// Some options are frontend flags (e.g.: -analyzer-output), but some are
+/// analyzer configuration options, which are preceded by -analyzer-config
+/// (e.g.: -analyzer-config notes-as-events=true).
+///
+/// If you'd like to add a new frontend flag, add it to
+/// include/clang/Driver/CC1Options.td, add a new field to store the value of
+/// that flag in this class, and initialize it in
+/// lib/Frontend/CompilerInvocation.cpp.
+///
+/// If you'd like to add a new non-checker configuration, register it in
+/// include/clang/StaticAnalyzer/Core/AnalyzerOptions.def, and refer to the
+/// top of the file for documentation.
+///
+/// If you'd like to add a new checker option, call getChecker*Option()
+/// whenever.
+///
+/// Some of the options are controlled by raw frontend flags for no good
reason,
+/// and should be eventually converted into -analyzer-config flags. New
analyzer
+/// options should not be implemented as frontend flags. Frontend flags still
+/// make sense for things that do not affect the actual analysis.
class AnalyzerOptions : public RefCountedBase {
public:
using ConfigTable = llvm::StringMap;
@@ -209,32 +231,21 @@ private:
#undef ANALYZER_OPTIO
[PATCH] D53483: [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346113: [analyzer] Restrict AnalyzerOptions' interface
so that non-checker objects have… (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D53483?vs=171060&id=172540#toc
Repository:
rL LLVM
https://reviews.llvm.org/D53483
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
cfe/trunk/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
cfe/trunk/unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -137,6 +137,28 @@
UMK_Deep = 2
};
+/// Stores options for the analyzer from the command line.
+///
+/// Some options are frontend flags (e.g.: -analyzer-output), but some are
+/// analyzer configuration options, which are preceded by -analyzer-config
+/// (e.g.: -analyzer-config notes-as-events=true).
+///
+/// If you'd like to add a new frontend flag, add it to
+/// include/clang/Driver/CC1Options.td, add a new field to store the value of
+/// that flag in this class, and initialize it in
+/// lib/Frontend/CompilerInvocation.cpp.
+///
+/// If you'd like to add a new non-checker configuration, register it in
+/// include/clang/StaticAnalyzer/Core/AnalyzerOptions.def, and refer to the
+/// top of the file for documentation.
+///
+/// If you'd like to add a new checker option, call getChecker*Option()
+/// whenever.
+///
+/// Some of the options are controlled by raw frontend flags for no good reason,
+/// and should be eventually converted into -analyzer-config flags. New analyzer
+/// options should not be implemented as frontend flags. Frontend flags still
+/// make sense for things that do not affect the actual analysis.
class AnalyzerOptions : public RefCountedBase {
public:
using ConfigTable = llvm::StringMap;
@@ -209,32 +231,21 @@
#undef ANALYZER_OPTION
#undef ANALYZER_OPTION_DEPENDS_ON_USER_MODE
- /// A helper function that retrieves option for a given full-qualified
- /// checker name.
- /// Options for checkers can be specified via 'analyzer-config' command-line
- /// option.
- /// Example:
- /// @code-analyzer-config unix.Malloc:OptionName=CheckerOptionValue @endcode
- /// or @code-analyzer-config unix:OptionName=GroupOptionValue @endcode
- /// for groups of checkers.
- /// @param [in] CheckerName Full-qualified checker name, like
- /// alpha.unix.StreamChecker.
- /// @param [in] OptionName Name of the option to get.
- /// @param [in] Default Default value if no option is specified.
- /// @param [in] SearchInParents If set to true and the searched option was not
- /// specified for the given checker the options for the parent packages will
- /// be searched as well. The inner packages take precedence over the outer
- /// ones.
- /// @retval CheckerOptionValue An option for a checker if it was specified.
- /// @retval GroupOptionValue An option for group if it was specified and no
- /// checker-specific options were found. The closer group to checker,
- /// the more priority it has. For example, @c coregroup.subgroup has more
- /// priority than @c coregroup for @c coregroup.subgroup.CheckerName checker.
- /// @retval Default If nor checker option, nor group option was found.
- StringRef getCheckerOption(StringRef CheckerName, StringRef OptionName,
- StringRef Default,
- bool SearchInParents = false);
+ /// Query an option's string value.
+ ///
+ /// If an option value is not provided, returns the given \p DefaultVal.
+ /// @param [in] Name Name for option to retrieve.
+ /// @param [in] DefaultVal Default value returned if no such option was
+ /// specified.
+ StringRef getStringOption(StringRef OptionName, StringRef DefaultVal);
+
+ void initOption(Optional &V, StringRef Name,
+ StringRef Def
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
NoQ added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); aaron.ballman wrote: > NoQ wrote: > > aaron.ballman wrote: > > > Szelethus wrote: > > > > Szelethus wrote: > > > > > ZaMaZaN4iK wrote: > > > > > > lebedev.ri wrote: > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > lebedev.ri wrote: > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > lebedev.ri wrote: > > > > > > > > > > > ZaMaZaN4iK wrote: > > > > > > > > > > > > aaron.ballman wrote: > > > > > > > > > > > > > `const auto *` > > > > > > > > > > > > Why do we need this change here? If I understand > > > > > > > > > > > > correctly, with `const auto*` we also need change > > > > > > > > > > > > initializer to > > > > > > > > > > > > `C.getSVal(CE->getSubExpr()).getAs().getPointer()`. > > > > > > > > > > > > But I don't understand why we need this. > > > > > > > > > > > Is `ValueToCastOptional` a pointer, a reference, or just > > > > > > > > > > > an actual `DefinedOrUnknownSVal`? I can't tell. > > > > > > > > > > > (sidenote: would be great to have a clang-tidy check for > > > > > > > > > > > this.) > > > > > > > > > > `ValueToCastOptional` is > > > > > > > > > > `llvm::Optional` > > > > > > > > > See, all my guesses were wrong. That is why it should not be > > > > > > > > > `auto` at all. > > > > > > > > I don't agree with you for this case. Honestly it's like a yet > > > > > > > > another holywar question. If we are talking only about this > > > > > > > > case - here you can see `getAs` part of > > > > > > > > the expression. this means clearly (at least for me) that we > > > > > > > > get something like `DefinedOrUnknownSVal`. What we get? I just > > > > > > > > press hotkey in my favourite IDE/text editor and see that > > > > > > > > `getAs` returns `llvm::Optional`. From my > > > > > > > > point of view it's clear enough here. > > > > > > > > > > > > > > > > If we are talking more generally about question "When should we > > > > > > > > use `auto` at all? " - we can talk, but not here, I think :) > > > > > > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable > > > > > > > comes to mind. > > > > > > > > What we get? I just press hotkey in my favourite IDE/text > > > > > > > > editor and see that getAs returns > > > > > > > > llvm::Optional > > > > > > > Which hotkey do i need to press to see this here, in the > > > > > > > phabricator? > > > > > > > > > > > > > > This really shouldn't be `auto`, if you have to explain that in > > > > > > > the variable's name, justify it in review comments. > > > > > > Ok, didn't know about such LLVM coding standard. Of course, with > > > > > > this information I will fix using `auto` here. Thank you. > > > > > Actually, I disagree. In the Static Analyzer we use `auto` if the > > > > > return type is in the name of the expression, and `getAs` may fail, > > > > > so it returns an `Optional`. In the case where a nullptr may be > > > > > returned to signal failure, `auto *` is used, so I believe that > > > > > `auto` is appropriate here. > > > > But don't change it back now, it doesn't matter a whole lot :D > > > > Actually, I disagree. In the Static Analyzer we use auto if the return > > > > type is in the name of the expression, and getAs may fail, so it > > > > returns an Optional. > > > > > > The static analyzer should be following the same coding standard as the > > > rest of the project. This is new code, so it's not matching inappropriate > > > styles from older code, so there's really no reason to not match the > > > coding standard. > > > > > > > In the case where a nullptr may be returned to signal failure, auto * > > > > is used, so I believe that auto is appropriate here. > > > > > > I disagree that `auto` is appropriate here. The amount of confusion about > > > the type demonstrates why. > > I confirm the strong local tradition of preferring `auto` for optional > > `SVal`s in new code, and i believe it's well-justified from the overall > > coding guideline's point of view. Even if you guys didn't get it right from > > the start, the amount of Analyzer-specific experience required to > > understand that it's an optional is very minimal and people get used to it > > very quickly when they start actively working on the codebase. > > > > On one hand, being a class that combines LLVM custom RTTI with > > pass-by-value semantics (i.e., it's like `QualType` when it comes to > > passing it around and like `Type` when it comes to casting), optionals are > > inevitable for representing `SVal` dynamic cast results. > > > > On the other hand, `SVal` is one of the most common classes of objects in > > the Static Analyzer (like, maybe, `Stmt` in Clang; i think a lot of people > > who are interested in Static Analyzer mor
[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Great! https://reviews.llvm.org/D54048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
