[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