[clang-tools-extra] r212540 - [clang-tidy] Address review comments for the Twine checker.
Alexander Kornienko
alexfh at google.com
Tue Jul 8 09:19:17 PDT 2014
On Tue, Jul 8, 2014 at 5:04 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
> On Tue, Jul 8, 2014 at 5:58 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
> > Thanks! See one nit below.
> >
> > On Tue, Jul 8, 2014 at 4:41 PM, Benjamin Kramer <
> benny.kra at googlemail.com>
> > wrote:
> >>
> >> Author: d0k
> >> Date: Tue Jul 8 10:41:20 2014
> >> New Revision: 212540
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=212540&view=rev
> >> Log:
> >> [clang-tidy] Address review comments for the Twine checker.
> >>
> >> - Remove unused includes.
> >> - Minor wording fix.
> >> - Added support to check for clang-tidy messages to
> >> check_clang_tidy_fix.sh
> >> = Updated test case.
> >>
> >> Modified:
> >> clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp
> >> clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h
> >> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh
> >> clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp
> >>
> >> Modified: clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp?rev=212540&r1=212539&r2=212540&view=diff
> >>
> >>
> ==============================================================================
> >> --- clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp
> (original)
> >> +++ clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.cpp Tue Jul
> 8
> >> 10:41:20 2014
> >> @@ -11,7 +11,6 @@
> >> #include "clang/AST/ASTContext.h"
> >> #include "clang/ASTMatchers/ASTMatchers.h"
> >> #include "clang/Lex/Lexer.h"
> >> -#include "llvm/Support/raw_ostream.h"
> >>
> >> using namespace clang::ast_matchers;
> >>
> >> @@ -29,7 +28,7 @@ void TwineLocalCheck::registerMatchers(M
> >> void TwineLocalCheck::check(const MatchFinder::MatchResult &Result) {
> >> const VarDecl *VD = Result.Nodes.getNodeAs<VarDecl>("variable");
> >> auto Diag = diag(VD->getLocation(),
> >> - "twine variables are prone to use after free bugs");
> >> + "twine variables are prone to use-after-free bugs");
> >>
> >> // If this VarDecl has an initializer try to fix it.
> >> if (VD->hasInit()) {
> >>
> >> Modified: clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h?rev=212540&r1=212539&r2=212540&view=diff
> >>
> >>
> ==============================================================================
> >> --- clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h (original)
> >> +++ clang-tools-extra/trunk/clang-tidy/llvm/TwineLocalCheck.h Tue Jul 8
> >> 10:41:20 2014
> >> @@ -11,7 +11,6 @@
> >> #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H
> >>
> >> #include "../ClangTidy.h"
> >> -#include "llvm/Support/Regex.h"
> >>
> >> namespace clang {
> >> namespace tidy {
> >>
> >> Modified:
> clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh?rev=212540&r1=212539&r2=212540&view=diff
> >>
> >>
> ==============================================================================
> >> --- clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh
> >> (original)
> >> +++ clang-tools-extra/trunk/test/clang-tidy/check_clang_tidy_fix.sh Tue
> >> Jul 8 10:41:20 2014
> >> @@ -7,5 +7,6 @@ CHECK_TO_RUN=$2
> >> TEMPORARY_FILE=$3.cpp
> >>
> >> grep -Ev "// *[A-Z-]+:" ${INPUT_FILE} > ${TEMPORARY_FILE}
> >> -clang-tidy ${TEMPORARY_FILE} -fix --checks="-*,${CHECK_TO_RUN}" --
> >> --std=c++11
> >> +clang-tidy ${TEMPORARY_FILE} -fix --checks="-*,${CHECK_TO_RUN}" --
> >> --std=c++11 > ${TEMPORARY_FILE}.msg 2>&1
> >> FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE}
> -strict-whitespace
> >> +not grep CHECK-MESSAGES ${INPUT_FILE} || FileCheck
> >> -input-file=${TEMPORARY_FILE}.msg ${INPUT_FILE}
> -check-prefix=CHECK-MESSAGES
> >
> >
> > Any reason to use "not X || Y" instead of "X && Y" or even "if X ; then
> Y ;
> > fi"?
>
> I care about the return value.
You're right. But it seems like we've started ignoring the return value of
the first FileCheck. We either need to use "bash -e" or add "|| exit $?".
> X && Y would return failure if X
> failed, I want it to succeed if X failed or Y passed so the other
> tests keep functioning.
>
> I guess an explicit if-then would be doable. Have to check how return
> values work when doing that.
>
They should work fine in ifs. In any case you could use explicit "exit".
>
> - Ben
>
> >
> >>
> >>
> >> Modified: clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp?rev=212540&r1=212539&r2=212540&view=diff
> >>
> >>
> ==============================================================================
> >> --- clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp
> >> (original)
> >> +++ clang-tools-extra/trunk/test/clang-tidy/llvm-twine-local.cpp Tue Jul
> >> 8 10:41:20 2014
> >> @@ -1,7 +1,5 @@
> >> -// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
> >> -// RUN: clang-tidy %t.cpp -checks='-*,llvm-twine-local' -fix -- >
> %t.msg
> >> 2>&1
> >> -// RUN: FileCheck -input-file=%t.cpp %s
> >> -// RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s
> >> +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s llvm-twine-local %t
> >> +// REQUIRES: shell
> >>
> >> namespace llvm {
> >> class Twine {
> >> @@ -17,19 +15,19 @@ using namespace llvm;
> >> void foo(const Twine &x);
> >>
> >> static Twine Moo = Twine("bark") + "bah";
> >> -// CHECK-MASSAGES: twine variables are prone to use after free bugs
> >> +// CHECK-MASSAGES: twine variables are prone to use-after-free bugs
> >> // CHECK-MESSAGES: note: FIX-IT applied suggested code changes
> >> // CHECK: static std::string Moo = (Twine("bark") + "bah").str();
> >>
> >> int main() {
> >> const Twine t = Twine("a") + "b" + Twine(42);
> >> -// CHECK-MASSAGES: twine variables are prone to use after free bugs
> >> +// CHECK-MASSAGES: twine variables are prone to use-after-free bugs
> >> // CHECK-MESSAGES: note: FIX-IT applied suggested code changes
> >> // CHECK: std::string t = (Twine("a") + "b" + Twine(42)).str();
> >> foo(Twine("a") + "b");
> >>
> >> Twine Prefix = false ? "__INT_FAST" : "__UINT_FAST";
> >> -// CHECK-MASSAGES: twine variables are prone to use after free bugs
> >> +// CHECK-MASSAGES: twine variables are prone to use-after-free bugs
> >> // CHECK-MESSAGES: note: FIX-IT applied suggested code changes
> >> // CHECK: const char * Prefix = false ? "__INT_FAST" : "__UINT_FAST";
> >> }
> >>
> >>
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140708/7f810946/attachment.html>
More information about the cfe-commits
mailing list