r348070 - Specify constant context in constant emitter

[Bill Wendling via cfe-commits]

Author: void
Date: Sat Dec  1 00:29:36 2018
New Revision: 348070

URL: http://llvm.org/viewvc/llvm-project?rev=348070&view=rev
Log:
Specify constant context in constant emitter

The constant emitter may need to evaluate the expression in a constant context.
For exasmple, global initializer lists.

Added:
cfe/trunk/test/CodeGen/builtin-constant-p.c
Modified:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/CodeGen/CGExprConstant.cpp
cfe/trunk/lib/CodeGen/ConstantEmitter.h
cfe/trunk/test/Analysis/builtin-functions.cpp

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=348070&r1=348069&r2=348070&view=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Sat Dec  1 00:29:36 2018
@@ -8199,6 +8199,7 @@ bool IntExprEvaluator::VisitBuiltinCallE
   // We can delay calculation of __builtin_constant_p until after
   // inlining. Note: This diagnostic won't be shown to the user.
   Info.FFDiag(E, diag::note_invalid_subexpr_in_const_expr);
+  return false;
 }
 return Success(false, E);
   }

Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprConstant.cpp?rev=348070&r1=348069&r2=348070&view=diff
==
--- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Sat Dec  1 00:29:36 2018
@@ -1455,6 +1455,7 @@ llvm::Constant *ConstantEmitter::tryEmit
 if (CD->isTrivial() && CD->isDefaultConstructor())
   return CGM.EmitNullConstant(D.getType());
   }
+  InConstantContext = true;
   }
 
   QualType destType = D.getType();
@@ -1552,7 +1553,7 @@ llvm::Constant *ConstantEmitter::tryEmit
   if (destType->isReferenceType())
 Success = E->EvaluateAsLValue(Result, CGM.getContext());
   else
-Success = E->EvaluateAsRValue(Result, CGM.getContext());
+Success = E->EvaluateAsRValue(Result, CGM.getContext(), InConstantContext);
 
   llvm::Constant *C;
   if (Success && !Result.HasSideEffects)

Modified: cfe/trunk/lib/CodeGen/ConstantEmitter.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ConstantEmitter.h?rev=348070&r1=348069&r2=348070&view=diff
==
--- cfe/trunk/lib/CodeGen/ConstantEmitter.h (original)
+++ cfe/trunk/lib/CodeGen/ConstantEmitter.h Sat Dec  1 00:29:36 2018
@@ -38,6 +38,9 @@ private:
   /// Whether the constant-emission failed.
   bool Failed = false;
 
+  /// Whether we're in a constant context.
+  bool InConstantContext = false;
+
   /// The AST address space where this (non-abstract) initializer is going.
   /// Used for generating appropriate placeholders.
   LangAS DestAddressSpace;

Modified: cfe/trunk/test/Analysis/builtin-functions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/builtin-functions.cpp?rev=348070&r1=348069&r2=348070&view=diff
==
--- cfe/trunk/test/Analysis/builtin-functions.cpp (original)
+++ cfe/trunk/test/Analysis/builtin-functions.cpp Sat Dec  1 00:29:36 2018
@@ -70,14 +70,14 @@ void test_constant_p() {
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning 
{{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning 
{{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning 
{{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning 
{{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // 
expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // 
expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning 
{{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning 
{{TRUE}}
 }

Added: cfe/trunk/test/CodeGen/builtin-constant-p.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=348070&view=auto
==
--- cfe/trunk/test/CodeGen/builtin-constant-p.c (added)
+

r348071 - Correct indentation.

[Bill Wendling via cfe-commits]

Author: void
Date: Sat Dec  1 01:06:26 2018
New Revision: 348071

URL: http://llvm.org/viewvc/llvm-project?rev=348071&view=rev
Log:
Correct indentation.

Modified:
cfe/trunk/lib/CodeGen/CGExprConstant.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprConstant.cpp?rev=348071&r1=348070&r2=348071&view=diff
==
--- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Sat Dec  1 01:06:26 2018
@@ -1455,7 +1455,7 @@ llvm::Constant *ConstantEmitter::tryEmit
 if (CD->isTrivial() && CD->isDefaultConstructor())
   return CGM.EmitNullConstant(D.getType());
   }
-  InConstantContext = true;
+InConstantContext = true;
   }
 
   QualType destType = D.getType();


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r348070 - Specify constant context in constant emitter

[Richard Smith via cfe-commits]

This seems is wrong way to handle this, and seems likely to be unsound in
C++. We should be creating a ConstantExpr node wrapped around the
initializer instead.

On Sat, 1 Dec 2018, 00:32 Bill Wendling via cfe-commits <
cfe-commits@lists.llvm.org wrote:

> Author: void
> Date: Sat Dec  1 00:29:36 2018
> New Revision: 348070
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348070&view=rev
> Log:
> Specify constant context in constant emitter
>
> The constant emitter may need to evaluate the expression in a constant
> context.
> For exasmple, global initializer lists.
>
> Added:
> cfe/trunk/test/CodeGen/builtin-constant-p.c
> Modified:
> cfe/trunk/lib/AST/ExprConstant.cpp
> cfe/trunk/lib/CodeGen/CGExprConstant.cpp
> cfe/trunk/lib/CodeGen/ConstantEmitter.h
> cfe/trunk/test/Analysis/builtin-functions.cpp
>
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=348070&r1=348069&r2=348070&view=diff
>
> ==
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Sat Dec  1 00:29:36 2018
> @@ -8199,6 +8199,7 @@ bool IntExprEvaluator::VisitBuiltinCallE
>// We can delay calculation of __builtin_constant_p until after
>// inlining. Note: This diagnostic won't be shown to the user.
>Info.FFDiag(E, diag::note_invalid_subexpr_in_const_expr);
> +  return false;
>  }
>  return Success(false, E);
>}
>
> Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprConstant.cpp?rev=348070&r1=348069&r2=348070&view=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Sat Dec  1 00:29:36 2018
> @@ -1455,6 +1455,7 @@ llvm::Constant *ConstantEmitter::tryEmit
>  if (CD->isTrivial() && CD->isDefaultConstructor())
>return CGM.EmitNullConstant(D.getType());
>}
> +  InConstantContext = true;
>}
>
>QualType destType = D.getType();
> @@ -1552,7 +1553,7 @@ llvm::Constant *ConstantEmitter::tryEmit
>if (destType->isReferenceType())
>  Success = E->EvaluateAsLValue(Result, CGM.getContext());
>else
> -Success = E->EvaluateAsRValue(Result, CGM.getContext());
> +Success = E->EvaluateAsRValue(Result, CGM.getContext(),
> InConstantContext);
>
>llvm::Constant *C;
>if (Success && !Result.HasSideEffects)
>
> Modified: cfe/trunk/lib/CodeGen/ConstantEmitter.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ConstantEmitter.h?rev=348070&r1=348069&r2=348070&view=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/ConstantEmitter.h (original)
> +++ cfe/trunk/lib/CodeGen/ConstantEmitter.h Sat Dec  1 00:29:36 2018
> @@ -38,6 +38,9 @@ private:
>/// Whether the constant-emission failed.
>bool Failed = false;
>
> +  /// Whether we're in a constant context.
> +  bool InConstantContext = false;
> +
>/// The AST address space where this (non-abstract) initializer is
> going.
>/// Used for generating appropriate placeholders.
>LangAS DestAddressSpace;
>
> Modified: cfe/trunk/test/Analysis/builtin-functions.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/builtin-functions.cpp?rev=348070&r1=348069&r2=348070&view=diff
>
> ==
> --- cfe/trunk/test/Analysis/builtin-functions.cpp (original)
> +++ cfe/trunk/test/Analysis/builtin-functions.cpp Sat Dec  1 00:29:36 2018
> @@ -70,14 +70,14 @@ void test_constant_p() {
>const int j = 2;
>constexpr int k = 3;
>clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning
> {{TRUE}}
> -  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning
> {{TRUE}}
> +  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning
> {{UNKNOWN}}
>clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning
> {{TRUE}}
>clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning
> {{TRUE}}
> -  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); //
> expected-warning {{TRUE}}
> +  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); //
> expected-warning {{UNKNOWN}}
>clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); //
> expected-warning {{TRUE}}
>clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); //
> expected-warning {{TRUE}}
>clang_analyzer_eval(__builtin_constant_p(" ") == 1); //
> expected-warning {{TRUE}}
> -  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); //
> expected-warning {{TRUE}}
> +  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); //
> expected-warning {{UNKNOWN}}
>clang_analyzer

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp updated this revision to Diff 176247.
dkrupp added a comment.

-clang-format applied
-clang:: namespace qualifiers removed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55125/new/

https://reviews.llvm.org/D55125

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  test/clang-tidy/misc-redundant-expression.cpp


Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -106,6 +106,7 @@
 
 #define COND_OP_MACRO 9
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {
   int k = 0;
   k += (y < 0) ? x : x;
@@ -114,11 +115,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expressions 
are equivalent
   k += (y < 0) ? COND_OP_MACRO : COND_OP_MACRO;
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'true' and 'false' expressions 
are equivalent
+  k += (y < 0) ? COND_OP_MACRO + COND_OP_OTHER_MACRO : COND_OP_MACRO + 
COND_OP_OTHER_MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: 'true' and 'false' expressions 
are equivalent
 
   // Do not match for conditional operators with a macro and a const.
   k += (y < 0) ? COND_OP_MACRO : 9;
   // Do not match for conditional operators with expressions from different 
macros.
   k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO;
+  // Do not match for conditional operators when a macro is defined to another 
macro
+  k += (y < 0) ? COND_OP_MACRO : COND_OP_THIRD_MACRO;
   return k;
 }
 #undef COND_OP_MACRO
Index: clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -598,23 +598,56 @@
   return true;
 }
 
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM) {
+  if (T1.getLength() != T2.getLength())
+return false;
+  return strncmp(SM.getCharacterData(T1.getLocation()),
+ SM.getCharacterData(T2.getLocation()), T1.getLength()) == 0;
+}
+
+//Returns true if both LhsEpxr and RhsExpr are
+//macro expressions and they are expanded
+//from different macros.
 static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
 const Expr *RhsExpr,
 const ASTContext *AstCtx) {
   if (!LhsExpr || !RhsExpr)
 return false;
-
-  SourceLocation LhsLoc = LhsExpr->getExprLoc();
-  SourceLocation RhsLoc = RhsExpr->getExprLoc();
-
-  if (!LhsLoc.isMacroID() || !RhsLoc.isMacroID())
+  SourceRange Lsr = LhsExpr->getSourceRange();
+  SourceRange Rsr = RhsExpr->getSourceRange();
+  if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
 return false;
-
   const SourceManager &SM = AstCtx->getSourceManager();
   const LangOptions &LO = AstCtx->getLangOpts();
 
-  return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) ==
-  Lexer::getImmediateMacroName(RhsLoc, SM, LO));
+  std::pair LsrLocInfo =
+  SM.getDecomposedLoc(SM.getExpansionLoc(Lsr.getBegin()));
+  std::pair RsrLocInfo =
+  SM.getDecomposedLoc(SM.getExpansionLoc(Rsr.getBegin()));
+  const llvm::MemoryBuffer *MB = SM.getBuffer(LsrLocInfo.first);
+
+  const char *LTokenPos = MB->getBufferStart() + LsrLocInfo.second;
+  const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second;
+  Lexer LRawLex(SM.getLocForStartOfFile(LsrLocInfo.first), LO,
+   MB->getBufferStart(), LTokenPos, MB->getBufferEnd());
+  Lexer RRawLex(SM.getLocForStartOfFile(RsrLocInfo.first), LO,
+   MB->getBufferStart(), RTokenPos, MB->getBufferEnd());
+
+  Token LTok, RTok;
+  SourceLocation LLoc, RLoc;
+  int LRem, RRem;
+  do {//we compare the expressions token-by-token
+LRawLex.LexFromRawLexer(LTok);
+RRawLex.LexFromRawLexer(RTok);
+LLoc = LTok.getLocation();
+RLoc = RTok.getLocation();
+LRem = SM.getCharacterData(SM.getExpansionLoc(Lsr.getEnd())) -
+   SM.getCharacterData(LLoc);//remaining characters until the end of 
expr
+RRem = SM.getCharacterData(SM.getExpansionLoc(Rsr.getEnd())) -
+   SM.getCharacterData(RLoc);
+  } while (!LTok.is(tok::eof) && !RTok.is(tok::eof) &&
+   compareToks(LTok, RTok, SM) && LRem > 0 && RRem > 0);
+  return !((LRem == 0 && RRem == 0) && compareToks(LTok, RTok, SM));
 }
 
 static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,


Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -106,6 +106,7 @@
 
 #define COND_OP_MACRO 9
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {
   int k = 0;
   k += (y < 0) ? x : x;
@@ -114,11 +115,15 @@
   // CHECK-MESSAG

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp marked 3 inline comments as done.
dkrupp added a comment.

In D55125#1314788 , @JonasToth wrote:

> In D55125#1314741 , @Szelethus wrote:
>
> > @JonasToth this is the `Lexer` based expression equality check I talked 
> > about in D54757#1311516 .  The 
> > point of this patch is that the definition is a macro sure could be build 
> > dependent, but the macro name is not, so it wouldn't warn on the case I 
> > showed. What do you think?
>
>
> Yes, this approach is possible.
>  IMHO it is still a bug/redudant if you do the same thing on both paths and 
> warning on it makes the programmer aware of the fact. E.g. the macros might 
> have been something different before, but a refactoring made them equal and 
> resulted in this situation.


@JonasThoth: I see you point with refactoring, but let's imagine that the two  
macros COND_OP_MACRO and COND_OP_THIRD_MACRO are defined by compile time 
parameters. If the two macros are happened to be defined to the same value the 
user would get a warning, and if not she would not.  How would the user would 
"fix" her code in the first case? So if the macro names are different in the 
conditional expression, it is not a redundant expression because the macro 
names are compile time parameters (with just eventually same values).  This was 
the same logic the old test case were testing:  k += (y < 0) ? COND_OP_MACRO : 
COND_OP_OTHER_MACRO;




Comment at: test/clang-tidy/misc-redundant-expression.cpp:109
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {

JonasToth wrote:
> Could you please add a test where the macro is `undef`ed and redefined to 
> something else?
I am not sure what you exactly suggest here. It should not matter what 
COND_OP_THIRD_MACRO is defined to as we lexically compare tokens as they are 
written in the original source code.

Could you be a bit more specific what test case you would like to add? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55125/new/

https://reviews.llvm.org/D55125



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

In D55125#1315335 , @Szelethus wrote:

> I see your point, but here's why I think it isn't a bug: I like to see macros 
> as `constexpr` variables, and if I used those instead, I personally wouldn't 
> like to get a warning just because they have the same value. In C, silencing 
> such a warning isn't even really possible.
>
> Another interesting thought, @donat.nagy's check works by comparing actual 
> nodes of the AST, while this one would work with `Lexer`, but they almost 
> want to do the same thing, the only difference is that the first checks 
> whether two pieces of code are equivalent, and the second checks whether they 
> are the same. Maybe it'd be worth to extract the logic into a simple 
> `areStmtsEqual(const Stmt *S1, const Stmt *S2, bool ShouldCompareLexically = 
> false)` function, that would do AST based comparison if the last param is set 
> to false, and would use `Lexer` if set to true. After that, we could just add 
> command line options to both of these checks, to leave it up to the user to 
> choose in between them. Maybe folks who suffer from heavily macro-infested 
> code would rather bear the obvious performance hit `Lexer` causes for little 
> more precise warnings, and (for example) users of C++11 (because there are 
> few excuses not to prefer `constexpr` there) could run in AST mode.
>
> edit: I'm not actually all that sure about the performance hit. Just a guess.
>
> But I'm just thinking aloud really.


Wiring out the lexical comparison and AST based comparison sounds like an 
interesting idea, however IMHO such a setting is too coarse-grained on the file 
level.  My guess would be that it depends from expression (fragment) to 
expression fragment how you want to compare them: for macros lexical comparison 
is better, for arithmetic expressions with many parentheses AST based recursive 
comparison may be a better fit (as implemented also in this checker).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55125/new/

https://reviews.llvm.org/D55125



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55136: [OpenCL][Sema] Improve BuildResolvedCallExpr handling of builtins

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno accepted this revision.
riccibruno added a comment.
This revision is now accepted and ready to land.

Much better. LGTM with a small format nit.




Comment at: lib/Sema/SemaExpr.cpp:5556
 // Extract the return type from the (builtin) function pointer type.
-auto FnPtrTy = Context.getPointerType(FDecl->getType());
+// FIXME Several builtins still have setType in 
Sema::CheckBuiltinFunctionCall.
+//   One should review their definitions in Builtins.def to ensure they

This goes over 80 cols.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55136/new/

https://reviews.llvm.org/D55136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a reviewer: alexfh.
Szelethus added a comment.

*advanced summoning*


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54401/new/

https://reviews.llvm.org/D54401



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Alexei,

I had a chance to have a quick look into this on a borrowed Mac Book. Seems 
like the assertion which was related to the revert is not existent anymore, but 
a new assertion came in. Next week I'll have time to debug that and I'll come 
back to you with what I find.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44100/new/

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

leonardchan wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > rjmccall wrote:
> > > > Hmm.  So adding a signed integer to an unsigned fixed-point type always 
> > > > converts the integer to unsigned?  That's a little weird, but only 
> > > > because the fixed-point rule seems to be the other way.  Anyway, I 
> > > > assume it's not a bug in the spec.
> > > `handleFixedPointConversion` only calculates the result type of the 
> > > expression, not the calculation type. The final result type of the 
> > > operation will be the unsigned fixed-point type, but the calculation 
> > > itself will be done in a signed type with enough precision to represent 
> > > both the signed integer and the unsigned fixed-point type. 
> > > 
> > > Though, if the result ends up being negative, the final result is 
> > > undefined unless the type is saturating. I don't think there is a test 
> > > for the saturating case (signed integer + unsigned saturating 
> > > fixed-point) in the SaturatedAddition tests.
> > > `handleFixedPointConversion` only calculates the result type of the 
> > > expression, not the calculation type.
> > 
> > Right, I understand that, but the result type of the expression obviously 
> > impacts the expressive range of the result, since you can end up with a 
> > negative value.
> > 
> > Hmm.  With that said, if the general principle is to perform the operation 
> > with full precision on the original operand values, why are unsigned 
> > fixed-point operands coerced to their corresponding signed types *before* 
> > the operation?  This is precision-limiting if the unsigned representation 
> > doesn't use a padding bit.  That seems like a bug in the spec.
> > Hmm. With that said, if the general principle is to perform the operation 
> > with full precision on the original operand values, why are unsigned 
> > fixed-point operands coerced to their corresponding signed types *before* 
> > the operation? This is precision-limiting if the unsigned representation 
> > doesn't use a padding bit. That seems like a bug in the spec.
> 
> Possibly. When the standard mentions converting from signed to unsigned fixed 
> point, the only requirement involved is conservation of magnitude (the number 
> of integral bits excluding the sign)
> 
> ```
> when signed and unsigned fixed-point types are mixed, the unsigned type is 
> converted to the corresponding signed type, and this should go without loss 
> of magnitude
> ```
> 
> This is just speculation, but I'm under the impression that not as much 
> "attention" was given for unsigned types as for signed types since "`In the 
> embedded processor world, support for unsigned fixed-point data types is 
> rare; normally only signed fixed-point data types are supported`", but was 
> kept anyway since unsigned types are used a lot.
> 
> ```
> However, to disallow unsigned fixed-point arithmetic from programming 
> languages (in general, and from C in particular) based on this observation, 
> seems overly restrictive.
> ```
> 
> I also imagine that if the programmer needs more precision, the correct 
> approach would be to cast up to a type with a higher scale. The standard 
> seems to make an effort to expose as much in regards to the underlying fixed 
> point types as possible:
> 
> ```
> it should be possible to write fixed-point algorithms that are independent of 
> the actual fixed-point hardware support. This implies that a programmer (or a 
> running program) should have access to all parameters that define the 
> behavior of the underlying hardware (in other words: even if these parameters 
> are implementation-defined).
> ```
> 
> So the user would at least know that unsigned types may not have the same 
> scale as their corresponding signed types if the hardware handles them with 
> different scales.
> 
> Also added test for signed integer + unsigned saturating fixed point.
As long as we maintain the same typing behavior, does the standard permit us to 
just Do The Right Thing here and do the extended arithmetic with the original 
unsigned operand?  I'm sure there are some cases where we would produce a 
slightly different value than an implementation that does the coercion before 
the operation, but that might be permitted under the standard, and as you say, 
it would only affect some situations that it doesn't seem the standard has 
given much thought to.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware updated this revision to Diff 176256.
baloghadamsoftware added a comment.

Thank you for the review!

You are absolutely right, these error messages were not accurate and even 
misleading. Now I updated them. To achieve this I also had to separate the 
function `isOutOfRange()` into three different functions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53812/new/

https://reviews.llvm.org/D53812

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/iterator-range.cpp

Index: test/Analysis/iterator-range.cpp
===
--- test/Analysis/iterator-range.cpp
+++ test/Analysis/iterator-range.cpp
@@ -23,34 +23,13 @@
 
 void simple_bad_end(const std::vector &v) {
   auto i = v.end();
-  *i; // expected-warning{{Iterator accessed outside of its range}}
-}
-
-void simple_good_begin(const std::vector &v) {
-  auto i = v.begin();
-  if (i != v.begin()) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_good_begin_negated(const std::vector &v) {
-  auto i = v.begin();
-  if (!(i == v.begin())) {
-clang_analyzer_warnIfReached();
-*--i; // no-warning
-  }
-}
-
-void simple_bad_begin(const std::vector &v) {
-  auto i = v.begin();
-  *--i; // expected-warning{{Iterator accessed outside of its range}}
+  *i; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy(const std::vector &v) {
   auto i1 = v.end();
   auto i2 = i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void decrease(const std::vector &v) {
@@ -70,7 +49,7 @@
   auto i1 = v.end();
   auto i2 = i1;
   --i1;
-  *i2; // expected-warning{{Iterator accessed outside of its range}}
+  *i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase1(const std::vector &v) {
@@ -86,7 +65,7 @@
   auto i2 = i1;
   ++i1;
   if (i2 == v.end())
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void copy_and_increase3(const std::vector &v) {
@@ -94,7 +73,7 @@
   auto i2 = i1;
   ++i1;
   if (v.end() == i2)
-*i2; // expected-warning{{Iterator accessed outside of its range}}
+*i2; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 template 
@@ -116,14 +95,14 @@
 
 void bad_non_std_find(std::vector &V, int e) {
   auto first = nonStdFind(V.begin(), V.end(), e);
-  *first; // expected-warning{{Iterator accessed outside of its range}}
+  *first; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void tricky(std::vector &V, int e) {
   const auto first = V.begin();
   const auto comp1 = (first != V.end()), comp2 = (first == V.end());
   if (comp1)
-*first;
+*first; // no-warning
 }
 
 void loop(std::vector &V, int e) {
@@ -147,7 +126,7 @@
   auto i0 = --L.cend();
   L.push_back(n);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void good_pop_back(std::list &L, int n) {
@@ -159,7 +138,7 @@
 void bad_pop_back(std::list &L, int n) {
   auto i0 = --L.cend(); --i0;
   L.pop_back();
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void good_push_front(std::list &L, int n) {
@@ -172,7 +151,7 @@
   auto i0 = L.cbegin();
   L.push_front(n);
   --i0;
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void good_pop_front(std::list &L, int n) {
@@ -184,13 +163,13 @@
 void bad_pop_front(std::list &L, int n) {
   auto i0 = ++L.cbegin();
   L.pop_front();
-  *--i0; // expected-warning{{Iterator accessed outside of its range}}
+  --i0; // expected-warning{{Iterator decremented ahead of its valid range}}
 }
 
 void bad_move(std::list &L1, std::list &L2) {
   auto i0 = --L2.cend();
   L1 = std::move(L2);
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
 }
 
 void bad_move_push_back(std::list &L1, std::list &L2, int n) {
@@ -198,5 +177,25 @@
   L2.push_back(n);
   L1 = std::move(L2);
   ++i0;
-  *++i0; // expected-warning{{Iterator accessed outside of its range}}
+  *++i0; // expected-warning{{The past-the-end iterator dereferenced}}
+}
+
+void good_incr_begin(const std::list &L) {
+  auto i0 = L.begin();
+  ++i0; // no-warning
+}
+
+void bad_decr_begin(const std::list &L) {
+  auto i0 = L.begin();
+  --i0;  // expected-warning{{Iterator decremented ahead of its valid range}}
+}
+
+void good_decr_end(const std::list &L) {
+  auto i0 = L.end();
+  --i0; // no-warning
+}
+
+void bad_incr_end(const std::list &L) {
+  auto i0 = L.end();
+  ++i0;  //

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/NestedNameSpecifier.h:220
+  void print(raw_ostream &OS, const PrintingPolicy &Policy,
+ bool ResolveTemplateArguments = false) const;
 

Quuxplusone wrote:
> Peanut gallery says: Should `ResolveTemplateArguments` really be here, or 
> should it be a property of the `PrintingPolicy` the same way e.g. 
> `ConstantsAsWritten` is a property of the `PrintingPolicy`?  I don't know 
> what a `PrintingPolicy` is, really, but I know that I hate defaulted boolean 
> function parameters with a passion. ;)
> 
> Furthermore, I note that the doc-comment for `ConstantsAsWritten`, at line 
> ~207 of include/clang/AST/PrettyPrinter.h, is nonsensical and maybe someone 
> should give it some love. (That is totally not //your// problem, though.)
I think this feels like a printing policy decision and should live there.



Comment at: lib/Sema/SemaTemplate.cpp:3071
+  printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
+}
+return;

Quuxplusone wrote:
> Checking my understanding: Am I correct that this code currently does not 
> pretty-print
> 
> static_assert(std::is_same(), "");
> 
> (creating an object of the trait type and then using its constexpr `operator 
> bool` to convert it to bool)? This is a rare idiom and doesn't need to be 
> supported AFAIC.
I'm fine worrying about that situation for a follow-up patch if it isn't 
currently supported.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54903/new/

https://reviews.llvm.org/D54903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

P-p-power ping!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54450/new/

https://reviews.llvm.org/D54450



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55101: [clang-tidy] Ignore namespaced and C++ member functions in google-objc-function-naming check 🙈

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D55101#1315294 , @benhamilton wrote:

> > Would you be okay with landing this fix and if we get any further reports 
> > for Objective-C++ sources then we can suppress it in all C++/Objective-C++ 
> > sources? I think there is enough value to enforcing the naming conventions 
> > on non-namespaced C functions in Objective-C++ to justify a simple followup 
> > fix. If other issues are reported after this then I also agree that 
> > enforcement in Objective-C++ sources may incur more overhead than it's 
> > worth.
>
> I'm not against it, but we've already disabled the majority of Objective-C 
> checks for Objective-C++ code, so I don't think this one should apply either.


I don't do a lot of Objective-C programming, so take my perspective with the 
giant grain of salt it deserves. I think this is a reasonable incremental 
improvement because this code seems more Cish than C++ish, so I can see an 
argument being made for following the Objective-C rules more than the C++ rules 
in this one instance. I think it's reasonable to push this commit and then 
revisit the question for the google module as a whole if there are conflicts 
between the rules in the same module.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55101/new/

https://reviews.llvm.org/D55101



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

Hi Gabor,
I think the code looks good. Thank you!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53708/new/

https://reviews.llvm.org/D53708



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Thank you for digging into this! Unfortunately, I don't have any MacOS device 
and I cannot check my code for the compatibility.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D44100/new/

https://reviews.llvm.org/D44100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53699/new/

https://reviews.llvm.org/D53699



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved 
into ImportDeclParts.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53755/new/

https://reviews.llvm.org/D53755



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55129: [CTU] Eliminate race condition in CTU lit tests

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Yes, the `%T` variable is obsolete, but it looks like the Guide recommendations 
are not entirely fulfilled.




Comment at: test/Analysis/ctu-main.cpp:1
-// RUN: mkdir -p %T/ctudir
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%T/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%T/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
-// RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-verify %s
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp

The Testing Infrastructure Guide recommends to use `rm -rf %t && mkdir %t`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55129/new/

https://reviews.llvm.org/D55129



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55132: [CTU] Add asserts to protect invariants

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.
This revision is now accepted and ready to land.

More assertions are always good.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55132/new/

https://reviews.llvm.org/D55132



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55133: [CTU] Add statistics

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Sorry, but I don't understand the meaning of some options. Could you please 
explain what are NumNoUnit and NumNotInOtherTU and what is the difference 
between them?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55133/new/

https://reviews.llvm.org/D55133



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55131: [CTU] Add more lit tests and better error handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Please find my comments inline.




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:251
+  cast_or_null(Importer.Import(const_cast(FD)));
+  if (!ToDecl) {
+return llvm::make_error(index_error_code::failed_import);

Conditionals with a single-line body don't require braces.



Comment at: test/Analysis/Inputs/ctu-other.c:1
+enum B {x = 42,l,s};
+

Please clang-format the new files.



Comment at: test/Analysis/Inputs/ctu-other.c:6
+  int b;
+} foobar;
+

Please use a consistent naming style across the file. There are names starting 
with capital, having underscores and written like this.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55131/new/

https://reviews.llvm.org/D55131



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55135: [CTU][Analyzer]Add DisplayCTUProgress analyzer switch

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
In addition to Umann remarks, there is a small comment inline.




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:239
+  if (DisplayCTUProgress) {
+llvm::errs() << "ANALYZE (CTU loaded AST for source file): "
+ << ASTFileName << "\n";

I think we can remove parens from the message.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55135/new/

https://reviews.llvm.org/D55135



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55134: [CTU] Add triple/lang mismatch handling

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,
Please find my comments inline.

> Hah. Do we support CTU for other languages, like ObjC and ObjC++? Can this be 
> an issue there?

That's a good question. In Samsung, CTU hasn't been tested on ObjC code. 
Upstream CTU supports only FunctionDecls as well so its ObjC status is 
questionable..




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:31
 
+namespace llvm {
+// Same as Triple's equality operator, but we check a field only if that is

Why don't we place it into the anon namespace just below?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+bool hasEqualKnownFields(const Triple &Lhs, const Triple &Rhs) {
+  return ((Lhs.getArch() != Triple::UnknownArch &&
+   Rhs.getArch() != Triple::UnknownArch)

This has to be split, probably with early returns. Example:
```if (Lhs.getArch() != Triple::UnknownArch && Rhs.getArch() != 
Triple::UnknownArch &&
  Lhs.getArch() != Rhs.getArch()
  return false;
...```



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:47
+  : true) &&
+ ((Lhs.getOS() != Triple::UnknownOS && Rhs.getOS() != 
Triple::UnknownOS)
+  ? Lhs.getOS() == Rhs.getOS()

We can use `Triple::isOSUnknown()` instead.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:59
+}
+}
+

Szelethus wrote:
> `// end of namespace llvm`
`// end namespace llvm` is much more common.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:203
 
+  const auto& TripleTo = Context.getTargetInfo().getTriple();
+  const auto& TripleFrom = Unit->getASTContext().getTargetInfo().getTriple();

Reference char should stick to the variable, not to the type.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:211
+// TODO pass the SourceLocation of the CallExpression for more precise
+// diagnostics
+Context.getDiagnostics().Report(diag::err_ctu_incompat_triple)

Comments should end with a dot. Same below.



Comment at: test/Analysis/ctu-unknown-parts-in-triples.cpp:8
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection  -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir 
-verify %s
+

Two spaces after `ExprInspection`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55134/new/

https://reviews.llvm.org/D55134



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55022: OpenCL: Extend argument promotion rules to vector types

[Matt Arsenault via Phabricator via cfe-commits]

arsenm closed this revision.
arsenm marked an inline comment as done.
arsenm added a comment.

r348083


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55022/new/

https://reviews.llvm.org/D55022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348083 - OpenCL: Extend argument promotion rules to vector types

[Matt Arsenault via cfe-commits]

Author: arsenm
Date: Sat Dec  1 13:56:10 2018
New Revision: 348083

URL: http://llvm.org/viewvc/llvm-project?rev=348083&view=rev
Log:
OpenCL: Extend argument promotion rules to vector types

The spec is ambiguous on whether vector types are allowed to be
implicitly converted. The only legal context I think this can
be used for OpenCL is printf, where it seems necessary.

Added:
cfe/trunk/test/CodeGenOpenCL/printf.cl
Modified:
cfe/trunk/lib/Sema/SemaExpr.cpp

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=348083&r1=348082&r2=348083&view=diff
==
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Sat Dec  1 13:56:10 2018
@@ -730,20 +730,33 @@ ExprResult Sema::DefaultArgumentPromotio
 return ExprError();
   E = Res.get();
 
+  QualType ScalarTy = Ty;
+  unsigned NumElts = 0;
+  if (const ExtVectorType *VecTy = Ty->getAs()) {
+NumElts = VecTy->getNumElements();
+ScalarTy = VecTy->getElementType();
+  }
+
   // If this is a 'float'  or '__fp16' (CVR qualified or typedef)
   // promote to double.
   // Note that default argument promotion applies only to float (and
   // half/fp16); it does not apply to _Float16.
-  const BuiltinType *BTy = Ty->getAs();
+  const BuiltinType *BTy = ScalarTy->getAs();
   if (BTy && (BTy->getKind() == BuiltinType::Half ||
   BTy->getKind() == BuiltinType::Float)) {
 if (getLangOpts().OpenCL &&
 !getOpenCLOptions().isEnabled("cl_khr_fp64")) {
-if (BTy->getKind() == BuiltinType::Half) {
-E = ImpCastExprToType(E, Context.FloatTy, CK_FloatingCast).get();
-}
+  if (BTy->getKind() == BuiltinType::Half) {
+QualType Ty = Context.FloatTy;
+if (NumElts != 0)
+  Ty = Context.getExtVectorType(Ty, NumElts);
+E = ImpCastExprToType(E, Ty, CK_FloatingCast).get();
+  }
 } else {
-  E = ImpCastExprToType(E, Context.DoubleTy, CK_FloatingCast).get();
+  QualType Ty = Context.DoubleTy;
+  if (NumElts != 0)
+Ty = Context.getExtVectorType(Ty, NumElts);
+  E = ImpCastExprToType(E, Ty, CK_FloatingCast).get();
 }
   }
 

Added: cfe/trunk/test/CodeGenOpenCL/printf.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/printf.cl?rev=348083&view=auto
==
--- cfe/trunk/test/CodeGenOpenCL/printf.cl (added)
+++ cfe/trunk/test/CodeGenOpenCL/printf.cl Sat Dec  1 13:56:10 2018
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=-+cl_khr_fp64 -triple 
spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck 
-check-prefixes=FP64,ALL %s
+// RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=-cl_khr_fp64 -triple 
spir-unknown-unknown -disable-llvm-passes -emit-llvm -o - %s | FileCheck 
-check-prefixes=NOFP64,ALL %s
+
+typedef __attribute__((ext_vector_type(2))) float float2;
+typedef __attribute__((ext_vector_type(2))) half half2;
+
+#ifdef cl_khr_fp64
+typedef __attribute__((ext_vector_type(2))) double double2;
+#endif
+
+int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 
2)));
+
+
+// ALL-LABEL: @test_printf_float2(
+// FP64: %conv = fpext <2 x float> %0 to <2 x double>
+// FP64: %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 
addrspace(2)* getelementptr inbounds ([5 x i8], [5 x i8] addrspace(2)* @.str, 
i32 0, i32 0), <2 x double> %conv)
+
+// NOFP64:  call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 
addrspace(2)* getelementptr inbounds ([5 x i8], [5 x i8] addrspace(2)* @.str, 
i32 0, i32 0), <2 x float> %0)
+kernel void test_printf_float2(float2 arg) {
+  printf("%v2f", arg);
+}
+
+// ALL-LABEL: @test_printf_half2(
+// FP64: %conv = fpext <2 x half> %0 to <2 x double>
+// FP64:  %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 
addrspace(2)* getelementptr inbounds ([5 x i8], [5 x i8] addrspace(2)* @.str, 
i32 0, i32 0), <2 x double> %conv) #2
+
+// NOFP64: %conv = fpext <2 x half> %0 to <2 x float>
+// NOFP64:  %call = call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 
addrspace(2)* getelementptr inbounds ([5 x i8], [5 x i8] addrspace(2)* @.str, 
i32 0, i32 0), <2 x float> %conv) #2
+kernel void test_printf_half2(half2 arg) {
+  printf("%v2f", arg);
+}
+
+#ifdef cl_khr_fp64
+// FP64-LABEL: @test_printf_double2(
+// FP64: call spir_func i32 (i8 addrspace(2)*, ...) @printf(i8 addrspace(2)* 
getelementptr inbounds ([5 x i8], [5 x i8] addrspace(2)* @.str, i32 0, i32 0), 
<2 x double> %0) #2
+kernel void test_printf_double2(double2 arg) {
+  printf("%v2f", arg);
+}
+#endif


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55023: OpenCL: Improve vector printf warnings

[Matt Arsenault via Phabricator via cfe-commits]

arsenm closed this revision.
arsenm added a comment.

r348084


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55023/new/

https://reviews.llvm.org/D55023



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r348084 - OpenCL: Improve vector printf warnings

[Matt Arsenault via cfe-commits]

Author: arsenm
Date: Sat Dec  1 14:16:27 2018
New Revision: 348084

URL: http://llvm.org/viewvc/llvm-project?rev=348084&view=rev
Log:
OpenCL: Improve vector printf warnings

The vector modifier is considered separate, so
don't treat it as a conversion specifier.

This is still not warning on some cases, like
using a type that isn't a valid vector element.

Fixes bug 39652

Added:
cfe/trunk/test/SemaOpenCL/format-strings-fixit.cl
Modified:
cfe/trunk/include/clang/AST/FormatString.h
cfe/trunk/lib/AST/FormatString.cpp
cfe/trunk/lib/AST/FormatStringParsing.h
cfe/trunk/lib/AST/PrintfFormatString.cpp
cfe/trunk/test/SemaOpenCL/printf-format-strings.cl

Modified: cfe/trunk/include/clang/AST/FormatString.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/FormatString.h?rev=348084&r1=348083&r2=348084&view=diff
==
--- cfe/trunk/include/clang/AST/FormatString.h (original)
+++ cfe/trunk/include/clang/AST/FormatString.h Sat Dec  1 14:16:27 2018
@@ -166,8 +166,6 @@ public:
 
 ZArg, // MS extension
 
-VArg, // OpenCL vectors
-
 // Objective-C specific specifiers.
 ObjCObjArg, // '@'
 ObjCBeg = ObjCObjArg,
@@ -305,6 +303,8 @@ public:
 
   QualType getRepresentativeType(ASTContext &C) const;
 
+  ArgType makeVectorType(ASTContext &C, unsigned NumElts) const;
+
   std::string getRepresentativeTypeName(ASTContext &C) const;
 };
 
@@ -324,6 +324,10 @@ public:
   : start(nullptr),length(0), hs(valid ? NotSpecified : Invalid), amt(0),
   UsesPositionalArg(0), UsesDotPrefix(0) {}
 
+  explicit OptionalAmount(unsigned Amount)
+: start(nullptr), length(0), hs(Constant), amt(Amount),
+UsesPositionalArg(false), UsesDotPrefix(false) {}
+
   bool isInvalid() const {
 return hs == Invalid;
   }
@@ -381,6 +385,8 @@ protected:
   LengthModifier LM;
   OptionalAmount FieldWidth;
   ConversionSpecifier CS;
+  OptionalAmount VectorNumElts;
+
   /// Positional arguments, an IEEE extension:
   ///  IEEE Std 1003.1, 2004 Edition
   ///  http://www.opengroup.org/onlinepubs/009695399/functions/printf.html
@@ -388,7 +394,8 @@ protected:
   unsigned argIndex;
 public:
   FormatSpecifier(bool isPrintf)
-: CS(isPrintf), UsesPositionalArg(false), argIndex(0) {}
+: CS(isPrintf), VectorNumElts(false),
+  UsesPositionalArg(false), argIndex(0) {}
 
   void setLengthModifier(LengthModifier lm) {
 LM = lm;
@@ -416,6 +423,14 @@ public:
 return FieldWidth;
   }
 
+  void setVectorNumElts(const OptionalAmount &Amt) {
+VectorNumElts = Amt;
+  }
+
+  const OptionalAmount &getVectorNumElts() const {
+return VectorNumElts;
+  }
+
   void setFieldWidth(const OptionalAmount &Amt) {
 FieldWidth = Amt;
   }
@@ -480,6 +495,9 @@ class PrintfSpecifier : public analyze_f
   OptionalFlag IsSensitive;  // '{sensitive}'
   OptionalAmount Precision;
   StringRef MaskType;
+
+  ArgType getScalarArgType(ASTContext &Ctx, bool IsObjCLiteral) const;
+
 public:
   PrintfSpecifier()
   : FormatSpecifier(/* isPrintf = */ true), HasThousandsGrouping("'"),

Modified: cfe/trunk/lib/AST/FormatString.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/FormatString.cpp?rev=348084&r1=348083&r2=348084&view=diff
==
--- cfe/trunk/lib/AST/FormatString.cpp (original)
+++ cfe/trunk/lib/AST/FormatString.cpp Sat Dec  1 14:16:27 2018
@@ -179,6 +179,36 @@ clang::analyze_format_string::ParseArgPo
 }
 
 bool
+clang::analyze_format_string::ParseVectorModifier(FormatStringHandler &H,
+  FormatSpecifier &FS,
+  const char *&I,
+  const char *E,
+  const LangOptions &LO) {
+  if (!LO.OpenCL)
+return false;
+
+  const char *Start = I;
+  if (*I == 'v') {
+++I;
+
+if (I == E) {
+  H.HandleIncompleteSpecifier(Start, E - Start);
+  return true;
+}
+
+OptionalAmount NumElts = ParseAmount(I, E);
+if (NumElts.getHowSpecified() != OptionalAmount::Constant) {
+  H.HandleIncompleteSpecifier(Start, E - Start);
+  return true;
+}
+
+FS.setVectorNumElts(NumElts);
+  }
+
+  return false;
+}
+
+bool
 clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,
   const char *&I,
   const char *E,
@@ -457,6 +487,14 @@ ArgType::matchesType(ASTContext &C, Qual
   llvm_unreachable("Invalid ArgType Kind!");
 }
 
+ArgType ArgType::makeVectorType(ASTContext &C, unsigned NumElts) const {
+  if (K != SpecificTy) // Won't be a valid vector element type.
+return ArgType::Invalid();
+
+  QualType Vec = C.getExtVectorType(T, NumElts);
+  return ArgType(Vec, Name);
+}
+
 QualType ArgType::getRepresentativeType(A

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk created this revision.
reuk added reviewers: Typz, krasimir, cfe-commits.

This patch aims to add support for the following rules from the JUCE coding 
standards:

- Always put a space before an open parenthesis that contains text - e.g. foo 
(123);
- Never put a space before an empty pair of open/close parenthesis - e.g. foo();

The entire set of JUCE coding guidelines can be found here 
. Unfortunately, 
clang-format can't enforce all of these style rules at the moment, so I'm 
trying to add support for them.

Patch by Reuben Thomas


Repository:
  rC Clang

https://reviews.llvm.org/D55170

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTest.cpp

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9271,6 +9271,59 @@
   verifyFormat("typedef void (*cb) (int);", Space);
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*(&a + 1);\n"
+   "&((&a)[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11055,6 +11108,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -62,7 +62,7 @@
 if (NonTemplateLess.count(CurrentToken->Previous))
   return false;
 
-const FormatToken &Previous = *CurrentToken->Previous;  // The '<'.
+const FormatToken &Previous = *CurrentToken->Previous; // The '<'.
 if (Previous.Previous) {
   if (Previous.Previous->Tok.isLiteral())
 return false;
@@ -366,7 +366,7 @@
   // specifier parameter, although this is technically valid:
   // [[foo(:)]]
   if (AttrTok->is(tok::colon) ||
-  AttrTok->startsSequence(tok::identifier, tok::identifier) || 
+  AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
 return false;
   if (AttrTok->is(tok::ellipsis))
@@ -523,11 +523,11 @@
 // Here, we set FirstObjCSelectorName when the end of the method call is
 // reached, in case it was not set already.
 if (!Contexts.back().FirstObjCSelectorName) {
-FormatToken* Previous = CurrentToken->getPr

[PATCH] D52967: Extend shelf-life by 70 years

[Bernhard M. Wiedemann via Phabricator via cfe-commits]

bmwiedemann added a comment.

As to the why: I'm working on reproducible builds for openSUSE and for that I 
verify that our packages can still give identical build results 15 years from 
now (which is the expected lifetime of today's enterprise software). `kvm -rtc 
base` option helps there.
Sometimes I even make it +20 years to find year-2038 bugs such as those in 
ninja and python , because it is better to 
fix them now rather than be surprised later and have a hard time with plenty 
busy work getting the fixes everywhere in time.

Now, it would be nice if someone could push this patch to the git repo.
I already got 200+ forks in my github home - most with just 1 small patch that 
eventually gets merged.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52967/new/

https://reviews.llvm.org/D52967



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits