[PATCH] D65549: [Sema] Prevent -Wunused-lambda-capture from generating false positive warnings on templated member function

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 214013.
ziangwan added a comment.

Fix style issue.


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

https://reviews.llvm.org/D65549

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp

Index: clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-lambda-capture -verify %s
+
+template 
+class TemplatedStatic {
+public:
+  TemplatedStatic() {
+[this](auto value) { Construct(value); }(5); // expected-warning {{lambda capture 'this' is not used}}
+  }
+
+  template 
+  static void Construct(Arg value) {}
+};
+
+template 
+class TemplatedMember {
+public:
+  TemplatedMember() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  template 
+  void Construct(Arg value) {}
+};
+
+template 
+class OverloadedStatic {
+public:
+  OverloadedStatic() {
+[this](auto value) { Construct(value); }(5); // expected-warning {{lambda capture 'this' is not used}}
+  }
+
+  static void Construct(int value) {}
+  static void Construct(float value) {}
+};
+
+template 
+class OverloadedMember {
+public:
+  OverloadedMember() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  void Construct(int value) {}
+  void Construct(float value) {}
+};
+
+template 
+class OverloadedMixTrueNegative {
+public:
+  OverloadedMixTrueNegative() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  void Construct(int value) {}
+  static void Construct(float value) {}
+};
+
+template 
+class OverloadedMixFalseNegative {
+public:
+  OverloadedMixFalseNegative() {
+// Currently we mark this capture as used whenever UnresolvedMemberExpr
+// happens, even if "Construct" will be resolved to the static one.
+// Ideally we should issue 'this' not used warning.
+[this](auto value) { Construct(value); }(5);
+  }
+
+  static void Construct(int value) {}
+  void Construct(float value) {}
+};
+
+int main() {
+  TemplatedStatic t1; // expected-note {{in instantiation of member function 'TemplatedStatic::TemplatedStatic' requested here}}
+  TemplatedMember t2;
+  OverloadedStatic t3; // expected-note {{in instantiation of member function 'OverloadedStatic::OverloadedStatic' requested here}}
+  OverloadedMember t4;
+  OverloadedMixTrueNegative t5;
+  OverloadedMixFalseNegative t6;
+  return 0;
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5619,6 +5619,14 @@
 *this, dyn_cast(Fn->IgnoreParens()),
 Fn->getBeginLoc());
 
+// Mark this capture as ODR-used if we are in a lambda
+// expression and the callee is UnresolvedMemberExpr.
+LambdaScopeInfo *CurLSI = getCurLambda();
+if (CurLSI && isa(Fn->IgnoreParens()))
+  for (auto  : CurLSI->Captures)
+if (Capture.isThisCapture())
+  Capture.markUsed(/*IsODRUse=*/true);
+
 return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
 VK_RValue, RParenLoc);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64666: [Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1613356 , @mgorny wrote:

> In D64666#1613057 , @ziangwan wrote:
>
> > The warning is actually correct here. This implicit integral to float 
> > conversion loses precision. Is it the intended behavior of the code? If so, 
> > we can simply add an explicit type cast to silence the warning.
>
>
> I don't really know. Hence, I added author of the code and libc++ maintainers 
> as subscribers. Do you think I should open a bug for it too?


We can wait for libc++ maintainers to respond before we open a bug.

Here is more insight of this warning:

  float a = unsigned int::max() - unsigned int::min() + float(1);

The `max() - min()` expression evaluates to 2147483645 as type unsigned int. It 
then gets converted to float type, loses precision and becomes 2147483648f, 
where the significand is all 1. Then, 1f is added to 2147483648f. The final 
value is 2147483648f because 1 is too small compared to 2147483648f and has no 
effect.

I don't know if the above is the intended behavior. Eventually, the final value 
is a floating point type with significand all ones. I guess that's what the 
author wants after all.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1612947 , @mgorny wrote:

> This change seems to have broken libc++ tests when run with trunk clang:
>  
> http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221/steps/run%20unit%20tests/logs/FAIL%3A%20libc%2B%2B%3A%3Aeval.pass.cpp
>  and more in http://lab.llvm.org:8011/builders/netbsd-amd64/builds/21221


The warning is actually correct here. This implicit integral to float 
conversion loses precision. Is it the intended behavior of the code? If so, we 
can simply add an explicit type cast to silence the warning.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

The two failing cases I am seeing are:

  /home/motus/netbsd8/netbsd8/llvm/projects/libcxx/include/random:3648:40: 
error: implicit conversion from 'unsigned long' to 'double' changes value from 
18446744073709551615 to 18446744073709551616 
[-Werror,-Wimplicit-int-float-conversion]
  const _RealType _Rp = _URNG::max() - _URNG::min() + _RealType(1);

  
/data/motus/netbsd8/netbsd8/llvm/projects/libcxx/test/std/numerics/rand/rand.util/rand.util.canonical/generate_canonical.pass.cpp:27:64:
 error: implicit conversion from 'unsigned int' to 'float' changes value from 
2147483645 to 2147483648 [-Werror,-Wimplicit-int-float-conversion]
  assert(f == truncate_fp((16807 - E::min()) / (E::max() - E::min() + 
F(1;

These are the intended behavior of this warning.

I will make a separate patch to fix the two failing cases.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

Fixed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

Buildbot failed for x86_64 target. Fix in progress.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

I have committed this patch. I will stay diligent in case anything fails.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D65549: [Sema] Prevent -Wunused-lambda-capture from generating false positive warnings on templated member function

[Ziang Wan via Phabricator via cfe-commits]

ziangwan created this revision.
ziangwan added reviewers: rsmith, nickdesaulniers, kongyi, pirama, stephenkelly.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This is a continuation of https://reviews.llvm.org/D44844 and a potential fix 
for https://bugs.llvm.org/show_bug.cgi?id=36880.

  template 
  class Variant {
  public:
Variant() {
  [this](auto value) { Construct(value); }(5); // false positive warning 
about "this" capture unused.
}
  
  
template 
void Construct(Arg value) {}
  };
  
  
  int main() {
Variant v;
return 0;
  }

Whenever we see a `UnresolvedMemberExpr`, we always mark a this capture 
ODR-used, even if later on it may be resolved to a static member function. The 
trade-off of this fix is that it will leads to false negative in the following 
case:

  template 
  class OverloadedMixFalseNegative {
  public:
OverloadedMixFalseNegative() {
  [this](auto value) { Construct(value); }(5); // warning disappears but 
should happen
}
  
static void Construct(int value) {}
void Construct(float value) {}
  };


Repository:
  rC Clang

https://reviews.llvm.org/D65549

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp

Index: clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-lambda-capture-this.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-lambda-capture -verify %s
+
+template 
+class TemplatedStatic {
+public:
+  TemplatedStatic() {
+[this](auto value) { Construct(value); }(5); // expected-warning {{lambda capture 'this' is not used}}
+  }
+
+  template 
+  static void Construct(Arg value) {}
+};
+
+template 
+class TemplatedMember {
+public:
+  TemplatedMember() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  template 
+  void Construct(Arg value) {}
+};
+
+template 
+class OverloadedStatic {
+public:
+  OverloadedStatic() {
+[this](auto value) { Construct(value); }(5); // expected-warning {{lambda capture 'this' is not used}}
+  }
+
+  static void Construct(int value) {}
+  static void Construct(float value) {}
+};
+
+template 
+class OverloadedMember {
+public:
+  OverloadedMember() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  void Construct(int value) {}
+  void Construct(float value) {}
+};
+
+template 
+class OverloadedMixTrueNegative {
+public:
+  OverloadedMixTrueNegative() {
+[this](auto value) { Construct(value); }(5);
+  }
+
+  void Construct(int value) {}
+  static void Construct(float value) {}
+};
+
+template 
+class OverloadedMixFalseNegative {
+public:
+  OverloadedMixFalseNegative() {
+// Currently we mark this capture as used whenever UnresolvedMemberExpr
+// happens, even if "Construct" will be resolved to the static one.
+// Ideally we should issue 'this' not used warning.
+[this](auto value) { Construct(value); }(5);
+  }
+
+  static void Construct(int value) {}
+  void Construct(float value) {}
+};
+
+int main() {
+  TemplatedStatic t1; // expected-note {{in instantiation of member function 'TemplatedStatic::TemplatedStatic' requested here}}
+  TemplatedMember t2;
+  OverloadedStatic t3; // expected-note {{in instantiation of member function 'OverloadedStatic::OverloadedStatic' requested here}}
+  OverloadedMember t4;
+  OverloadedMixTrueNegative t5;
+  OverloadedMixFalseNegative t6;
+  return 0;
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5619,6 +5619,17 @@
 *this, dyn_cast(Fn->IgnoreParens()),
 Fn->getBeginLoc());
 
+// Mark this capture as ODR-used if we are in a lambda
+// expression and the callee is UnresolvedMemberExpr.
+LambdaScopeInfo *CurLSI = getCurLambda();
+if (CurLSI && isa(Fn->IgnoreParens())) {
+  for (auto  : CurLSI->Captures) {
+if (Capture.isThisCapture()) {
+  Capture.markUsed(/*IsODRUse=*/true);
+}
+  }
+}
+
 return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
 VK_RValue, RParenLoc);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 2 inline comments as done.
ziangwan added inline comments.



Comment at: clang/test/SemaCXX/thread-safety-annotations.h:47
+// Enable thread safety attributes only with clang.
+// The attributes can be safely erased when compiling with other compilers.
+#if defined(__clang__) && (!defined(SWIG))

aaronpuchert wrote:
> ziangwan wrote:
> > aaronpuchert wrote:
> > > ziangwan wrote:
> > > > nickdesaulniers wrote:
> > > > > Is this test suite run with other compilers? If not, I think we can 
> > > > > remove the case?
> > > > Yeah, you are right. I just copied the header definitions from clang 
> > > > thread safety analysis doc.
> > > Why aren't you using the existing macros? The idea was to run the tests 
> > > with both old and new terminology, and for the time being, I think we 
> > > should maintain both.
> > There are already tests on existing macros. I want to introduce tests about 
> > the new macros as well.
> There are no old and new macros, there are only old and new **attributes**. 
> The existing macros use both sets of attributes, depending on the value of 
> `USE_CAPABILITY` with which the tests are run. Using other macro names is 
> just window dressing.
Gotcha.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:135-140
+  if (condition) {
+assertNotHeld(); // expected-warning {{mutex '!mu' is acquired exclusively 
and shared in the same scope}}
+  } else {
+mu.Lock();
+mu.Unlock(); // expected-warning {{the other acquisition of mutex '!mu' is 
here}}
+  }

aaronpuchert wrote:
> ziangwan wrote:
> > aaronpuchert wrote:
> > > ziangwan wrote:
> > > > aaronpuchert wrote:
> > > > > Why would I want these warnings here? This code seems fine to me.
> > > > > 
> > > > > However, I don't see why we don't get `acquiring mutex 'mu' requires 
> > > > > negative capability '!mu'` at line 138, or does that disappear 
> > > > > because of the assertion?
> > > > Showing `acquiring mutex 'mu' requires negative capability '!mu'` is 
> > > > not in the scope of this patch. Please notice thread safety analysis is 
> > > > still under development.
> > > > 
> > > > The reason is that, in one path we have 
> > > > `ASSERT_SHARED_CAPABILITY(!mu)`, and in the other path we have 
> > > > `RELEASE(mu)`. The assertion leads to negative shared capability but 
> > > > the release leads to negative exclusive capability. A merge of the two 
> > > > capabilities (merging "I am not trying to read" versus "I am not trying 
> > > > to write") leads to a warning.
> > > > 
> > > > Without my patch, clang will issue a warning for the merge point in 
> > > > test1() but not the merge point in test2().
> > > But `ASSERT_SHARED_CAPABILITY(!mu)` implies that we also don't have an 
> > > exclusive lock (an exclusive lock is stronger than a shared lock), and 
> > > `RELEASE(mu)` without `ACQUIRE_SHARED(mu)` implies that we have neither a 
> > > shared nor an exclusive lock as well.
> > > 
> > > In the end, I have the same question as above: Why do we want two kinds 
> > > of negative capabilities? Isn't the idea that negative capabilities 
> > > express the lock not being held at all?
> > The problem is: by the current state of the thread safety analysis, 
> > ASSERT_SHARED_CAPABILTIY(!mu) introduces a shared negative capability, 
> > whereas RELEASE(mu) and RELEASE_SHARED(mu) introduce an exclusive negative 
> > capability, and UNLOCK_FUNCTION(mu) introduces a generic negative 
> > capability. All three are different. At merge points, warnings will be 
> > issued if different types of negative capability are merged. The current 
> > thread safety analysis produces bogus false positive in our code base.
> > 
> > The solution I propose is that we should at least make RELEASE_SHARED(mu) 
> > produce a shared negative capability.
> > 
> > Regarding why we should have types for negative capability, thinking about 
> > "exclusive !mu" in a reader-writer lock situation, which means "I am not 
> > trying to write". However, the code can still read. In other words, 
> > "exclusive !mu" does not imply "shared !mu", and the code can still hold 
> > the lock in shared state. Without the types of negative capability, we 
> > wouldn't be able to express the situation described above.
> I'm not doubting that there is a bug, but I don't think this is the right 
> solution.
> 
> > ASSERT_SHARED_CAPABILTIY(!mu) introduces a shared negative capability
> We should disallow using "shared" attributes with negative capabilities, and 
> I'm surprised this isn't already the case.
> 
> > whereas RELEASE(mu) and RELEASE_SHARED(mu) introduce an exclusive negative 
> > capability [...] The solution I propose is that we should at least make 
> > RELEASE_SHARED(mu) produce a shared negative capability.
> Clearly both leave `mu` in the same (unlocked) state, so why distinguish 
> between them? After the lock is released, whichever mode it has been in 
> before is 

[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:135-140
+  if (condition) {
+assertNotHeld(); // expected-warning {{mutex '!mu' is acquired exclusively 
and shared in the same scope}}
+  } else {
+mu.Lock();
+mu.Unlock(); // expected-warning {{the other acquisition of mutex '!mu' is 
here}}
+  }

aaronpuchert wrote:
> ziangwan wrote:
> > aaronpuchert wrote:
> > > Why would I want these warnings here? This code seems fine to me.
> > > 
> > > However, I don't see why we don't get `acquiring mutex 'mu' requires 
> > > negative capability '!mu'` at line 138, or does that disappear because of 
> > > the assertion?
> > Showing `acquiring mutex 'mu' requires negative capability '!mu'` is not in 
> > the scope of this patch. Please notice thread safety analysis is still 
> > under development.
> > 
> > The reason is that, in one path we have `ASSERT_SHARED_CAPABILITY(!mu)`, 
> > and in the other path we have `RELEASE(mu)`. The assertion leads to 
> > negative shared capability but the release leads to negative exclusive 
> > capability. A merge of the two capabilities (merging "I am not trying to 
> > read" versus "I am not trying to write") leads to a warning.
> > 
> > Without my patch, clang will issue a warning for the merge point in test1() 
> > but not the merge point in test2().
> But `ASSERT_SHARED_CAPABILITY(!mu)` implies that we also don't have an 
> exclusive lock (an exclusive lock is stronger than a shared lock), and 
> `RELEASE(mu)` without `ACQUIRE_SHARED(mu)` implies that we have neither a 
> shared nor an exclusive lock as well.
> 
> In the end, I have the same question as above: Why do we want two kinds of 
> negative capabilities? Isn't the idea that negative capabilities express the 
> lock not being held at all?
The problem is: by the current state of the thread safety analysis, 
ASSERT_SHARED_CAPABILTIY(!mu) introduces a shared negative capability, whereas 
RELEASE(mu) and RELEASE_SHARED(mu) introduce an exclusive negative capability, 
and UNLOCK_FUNCTION(mu) introduces a generic negative capability. All three are 
different. At merge points, warnings will be issued if different types of 
negative capability are merged. The current thread safety analysis produces 
bogus false positive in our code base.

The solution I propose is that we should at least make RELEASE_SHARED(mu) 
produce a shared negative capability.

Regarding why we should have types for negative capability, thinking about 
"exclusive !mu" in a reader-writer lock situation, which means "I am not trying 
to write". However, the code can still read. In other words, "exclusive !mu" 
does not imply "shared !mu", and the code can still hold the lock in shared 
state. Without the types of negative capability, we wouldn't be able to express 
the situation described above.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184



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

[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/test/SemaCXX/thread-safety-annotations.h:47
+// Enable thread safety attributes only with clang.
+// The attributes can be safely erased when compiling with other compilers.
+#if defined(__clang__) && (!defined(SWIG))

aaronpuchert wrote:
> ziangwan wrote:
> > nickdesaulniers wrote:
> > > Is this test suite run with other compilers? If not, I think we can 
> > > remove the case?
> > Yeah, you are right. I just copied the header definitions from clang thread 
> > safety analysis doc.
> Why aren't you using the existing macros? The idea was to run the tests with 
> both old and new terminology, and for the time being, I think we should 
> maintain both.
There are already tests on existing macros. I want to introduce tests about the 
new macros as well.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184



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

[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

The problem is: by the current state of the thread safety analysis, 
`ASSERT_SHARED_CAPABILTIY(!mu)` introduces a shared negative capability, 
whereas `RELEASE(mu)` **and `RELEASE_SHARED(mu)`**  introduce an exclusive 
negative capability, and `UNLOCK_FUNCTION(mu)` introduces a generic negative 
capability. All three are different. At merge points, warnings will be issued 
if different types of negative capability are merged. The current thread safety 
analysis produces bogus false positive in our code base.

The solution I propose is that we should at least make `RELEASE_SHARED(mu)` 
produce a shared negative capability.

Regarding why we should have types for negative capability, thinking about 
"exclusive !mu" in a reader-writer lock situation, which means "I am not trying 
to write". However, the code can still read. In other words, "exclusive !mu" 
does not imply "shared !mu", and the code can still hold the lock in shared 
state. Without the types of negative capability, we wouldn't be able to express 
the situation described above.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:11656
+  IsListInit =
+  IsListInit || (isa(OrigE) && S.getLangOpts().CPlusPlus);
+

aaron.ballman wrote:
> ziangwan wrote:
> > aaron.ballman wrote:
> > > Do you want to perform the `isa<>` on `OrigE` or on `E` which has had 
> > > paren and implicit casts stripped?
> > When we are dealing with Initialization list syntax expression, the 
> > out-most expression will be of type `InitListExpr`. Since the out-most 
> > expression is not `ImplicitCastExpr`, `IgnoreParenImpCasts()` has no effect 
> > on the `OrigE` object. In other words, `OrigE` and `E` will be the same 
> > when `isa(OrigE)` evaluates to true.
> > 
> > In this case, I personally prefer `OrigE` since it is the "non-processed" 
> > object. However, the rest of the function uses `E` only. I can change it to 
> > `E`.
> Ah, I was thinking we had to contend with a case like `({1, 2, 3})` where 
> there's a `ParenExpr` involved, but I am not certain that's a case we have to 
> worry about.
I think `({1,2,3})` is not a valid initializer.
```
implicit-int-float-narrowing.cpp:6:16: warning: expression result unused 
[-Wunused-value]
  int a[3] = ({1,2,3});
   ^
implicit-int-float-narrowing.cpp:6:18: warning: expression result unused 
[-Wunused-value]
  int a[3] = ({1,2,3});
 ^
implicit-int-float-narrowing.cpp:6:21: error: expected ';' after expression
  int a[3] = ({1,2,3});
^
implicit-int-float-narrowing.cpp:6:7: error: array initializer must be an 
initializer list
  int a[3] = ({1,2,3});
  ^
```
If so, I don think we need to worry about there's a `ParenExpr` involved.



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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:11656
+  IsListInit =
+  IsListInit || (isa(OrigE) && S.getLangOpts().CPlusPlus);
+

aaron.ballman wrote:
> Do you want to perform the `isa<>` on `OrigE` or on `E` which has had paren 
> and implicit casts stripped?
When we are dealing with Initialization list syntax expression, the out-most 
expression will be of type `InitListExpr`. Since the out-most expression is not 
`ImplicitCastExpr`, `IgnoreParenImpCasts()` has no effect on the `OrigE` 
object. In other words, `OrigE` and `E` will be the same when 
`isa(OrigE)` evaluates to true.

In this case, I personally prefer `OrigE` since it is the "non-processed" 
object. However, the rest of the function uses `E` only. I can change it to `E`.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 212015.
ziangwan added a comment.

Fix typos.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-int-float-conversion.c
  clang/test/Sema/implicit-int-float-narrowing.cpp

Index: clang/test/Sema/implicit-int-float-narrowing.cpp
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-narrowing.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wno-c++11-narrowing -Wimplicit-int-float-conversion
+
+void testNoWarningOnNarrowing() {
+  // Test that we do not issue duplicated warnings for
+  // C++11 narrowing.
+  float a = {L}; // expected-no-diagnostics
+
+  long b = L;
+  float c = {b}; // expected-no-diagnostics
+}
Index: clang/test/Sema/implicit-int-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-conversion.c
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-int-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+
+#ifndef __ILP32__
+  float ff = L;// expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+#else
+  float ff = L;// expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from  to 1312}}
+#endif
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+
+#ifndef __ILP32__
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+#else
+  float b = L + a; // expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+#endif
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+
+  double e = 0.0;
+  double f = i + e;
+}
+
+void testCNarrowing() {
+  // Since this is a C file. C++11 narrowing is not in effect.
+  // In this case, we should issue warnings.
+#ifndef __ILP32__
+  float a = {L}; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+#else
+  float a = {L};   // expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+#endif
+
+  long b = L;
+  float c = {b}; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
-  
-  vd = l + vd;
-  vd = vd + t;
+
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 212001.
ziangwan added a comment.

Update diff.

1. Silence the warning when C++11 narrowing is in effect on intialization-list 
expression.
2. Add test cases to test "no duplicated warnings for c++11 narrowing"
3. Updated test cases to handle 32-bit host machine.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-int-float-conversion.c
  clang/test/Sema/implicit-int-float-narrowing.cpp

Index: clang/test/Sema/implicit-int-float-narrowing.cpp
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-narrowing.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wno-c++11-narrowing -Wimplicit-int-float-conversion
+
+void testNoWarningOnNarrowing() {
+  // Test that we do not issue duplicated warnings for
+  // C++11 narrowing.
+  float a = {L}; // expected-no-diagnostics
+
+  long b = L;
+  float c = {b}; // expected-no-diagnostics
+}
Index: clang/test/Sema/implicit-int-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-conversion.c
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-int-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+
+#ifndef __ILP32__
+  float ff = L;// expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+#else
+  float ff = L;// expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long long' to 'float' changes value from  to 1312}}
+#endif
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+
+#ifndef __ILP32__
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+#else
+  float b = L + a; // expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+#endif
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+
+  double e = 0.0;
+  double f = i + e;
+}
+
+void testCNarrowing() {
+  // Since this is a C file. C++11 narrowing is not in effect.
+  // In this case, we should issue warnings.
+#ifndef __ILP32__
+  float a = {L}; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+#else
+  float a = {L};   // expected-warning {{implicit conversion from 'long long' to 'float' changes value from  to 1312}}
+#endif
+
+  long b = L;
+  float c = {b}; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
-  
-  vd = l + vd;
-  vd = vd + t;
+
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from 

[PATCH] D65184: [Sema] Thread Safety Analysis: Fix negative capability's LockKind representation.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 4 inline comments as done.
ziangwan added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:2188-2190
+/// shared + exclusive = exclusive
+/// generic + exclusive = exclusive
+/// generic + shared = shared

aaronpuchert wrote:
> What do these lines mean? That we accept if a lock is shared in one branch 
> and exclusive in the other, and that we make it exclusive after the merge 
> point?
Yes. If at CFG merge point, one path holds type1 lock and the other path holds 
type2 lock.

We do a type1 + type2 = merged_type and issue warnings if we are doing shared + 
exclusive merge.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:2219-2221
+if (LDat1->kind() == LK_Generic || LDat2->kind() == LK_Generic) {
+  // No warning is issued in this case.
+  if (Modify && LDat1->kind() == LK_Generic) {

nickdesaulniers wrote:
> The double check of `LDat1->kind() == LK_Generic` is fishy to me.  
> Particularly the case where `LDat1->kind() == LK_Generic` is false but 
> `LDat2->kind() == LK_Generic` is true.
> 
> This might be clearer as:
> ```
> if (LDat2->kind() == LK_Generic)
>   continue;
> else if (LDat1->kind() == LK_Generic && Modify)
>   *Iter1 = Fact;
> else {
>   ...
> ```
> Or is there something else to this logic I'm missing?
I think your suggestion is to refactor the if statements. What I am thinking is 
that there are two cases.
1. One of the two locks is generic
  * If so, then take the type of the other non-generic lock (shared or 
exclusive).
2. Neither of the two locks is generic.

My if statement is trying express that. I am afraid the refactoring is going to 
lose the logic (as stated in my comment above).



Comment at: clang/test/SemaCXX/thread-safety-annotations.h:47
+// Enable thread safety attributes only with clang.
+// The attributes can be safely erased when compiling with other compilers.
+#if defined(__clang__) && (!defined(SWIG))

nickdesaulniers wrote:
> Is this test suite run with other compilers? If not, I think we can remove 
> the case?
Yeah, you are right. I just copied the header definitions from clang thread 
safety analysis doc.



Comment at: clang/test/SemaCXX/warn-thread-safety-negative.cpp:135-140
+  if (condition) {
+assertNotHeld(); // expected-warning {{mutex '!mu' is acquired exclusively 
and shared in the same scope}}
+  } else {
+mu.Lock();
+mu.Unlock(); // expected-warning {{the other acquisition of mutex '!mu' is 
here}}
+  }

aaronpuchert wrote:
> Why would I want these warnings here? This code seems fine to me.
> 
> However, I don't see why we don't get `acquiring mutex 'mu' requires negative 
> capability '!mu'` at line 138, or does that disappear because of the 
> assertion?
Showing `acquiring mutex 'mu' requires negative capability '!mu'` is not in the 
scope of this patch. Please notice thread safety analysis is still under 
development.

The reason is that, in one path we have `ASSERT_SHARED_CAPABILITY(!mu)`, and in 
the other path we have `RELEASE(mu)`. The assertion leads to negative shared 
capability but the release leads to negative exclusive capability. A merge of 
the two capabilities (merging "I am not trying to read" versus "I am not trying 
to write") leads to a warning.

Without my patch, clang will issue a warning for the merge point in test1() but 
not the merge point in test2().


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65184



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

My bad. I omit these two test cases. I apologize for my carelessness.

Yes, this warning definitely is a duplicate of c++11 narrowing, *when the code 
actually uses initialization-list syntax. For example, the following code would 
issue duplicated warnings for now:

  float ff = {L};  
  long a = L;
  float  = {a};

However, c++11 narrowing conversion does not handle these cases. It does not 
handle initialization without initialization-list syntax. It does not handle 
implicit conversion in expressions. It does not handle implicit conversion in 
argument passing as well.

  void test(float b) {}
  
  float f = L; // no narrowing warning
  
  long a = L;
  float fff = a; // warning
  float c = a + fff; // no narrowing warning of a from long to float
  
  test(a); // no narrowing warning

Handling extra cases allow us to sanity-check code that does not use 
initialization-list syntax or has implicit conversion in expressions. I believe 
it is a valuable addition. A fix would be to let 
`-Wimplicit-int-float-conversion` skip initialization-list syntax. The other 
way would to augment c++ narrowing. What do you guys think is the proper way?

I will continue work on this patch. Please don't panic.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 211666.
ziangwan added a comment.

Update diff.

1. Fix 2 failing test cases. I omitted these two. My mistake.
2. Fix trailing whitespaces.

Okay?


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
  clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-int-float-conversion.c

Index: clang/test/Sema/implicit-int-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-int-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+
+  float ff = L;// expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
-  
-  vd = l + vd;
-  vd = vd + t;
+
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v);  // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v);  // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
===
--- clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
+++ clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
@@ -121,9 +121,9 @@
 
   // Constants.
   Agg f4 = {12345678};  // OK 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1600354 , @phosek wrote:

> I'm seeing some test failures which appear to have been introduced by this 
> change:
>
>    TEST 'Clang :: 
> CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp' FAILED 
>   Script:
>   --
>   : 'RUN: at line 1';   
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 
> -internal-isystem 
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include
>  -nostdsysteminc -fsyntax-only -std=c++11 -triple x86_64-apple-macosx10.6.7 
> -verify 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
>   --
>   Exit Code: 1
>  
>   Command Output (stderr):
>   --
>   error: 'warning' diagnostics seen but not expected: 
> File 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
>  Line 136: implicit conversion from 'int' to 'float' changes value from 
> 123456789 to 123456792
> File 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-0x.cpp
>  Line 138: implicit conversion from 'int' to 'float' changes value from 
> 123456789 to 123456792
>   2 errors generated.
>  
>   --
>  
>   
>   Testing: 0 .
>   FAIL: Clang :: CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp 
> (1096 of 15293)
>    TEST 'Clang :: 
> CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp' FAILED 
> 
>   Script:
>   --
>   : 'RUN: at line 1';   
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 
> -internal-isystem 
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include
>  -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=c++11-narrowing -triple 
> x86_64-apple-macosx10.6.7 -verify 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
>   : 'RUN: at line 2';   
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/bin/clang -cc1 
> -internal-isystem 
> /b/s/w/ir/k/recipe_cleanup/clango5zn3b/llvm_build_dir/lib/clang/10.0.0/include
>  -nostdsysteminc -fsyntax-only -std=c++11 -Wno-error=narrowing -triple 
> x86_64-apple-macosx10.6.7 -verify 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
>   --
>   Exit Code: 1
>  
>   Command Output (stderr):
>   --
>   error: 'warning' diagnostics seen but not expected: 
> File 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
>  Line 124: implicit conversion from 'int' to 'float' changes value from 
> 123456789 to 123456792
> File 
> /b/s/w/ir/k/llvm-project/clang/test/CXX/dcl.decl/dcl.init/dcl.init.list/p7-cxx11-nowarn.cpp
>  Line 126: implicit conversion from 'int' to 'float' changes value from 
> 123456789 to 123456792
>   2 errors generated.
>  
>   --
>


This is the intended behavior of this warning. I will fix the test cases and 
add `expected-warning` to them.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 211617.
ziangwan added a comment.

Update diff.

1. Make the two warnings controlled by `-Wimplicit-int-float-conversion`, which 
is a subset of `-Wimplicit-float-conversion`.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-int-float-conversion.c

Index: clang/test/Sema/implicit-int-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-int-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-int-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,55 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy loss. 
+  if (SourceBT && 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1598194 , @jfb wrote:

> Thanks, the update looks good.
>
> One thing I just noticed: if I have a codebase with 
> `-Wimplicit-float-conversion` it seems like updating clang will automatically 
> turn the two new warnings on, correct? That seems good, but also difficult to 
> roll out because I can't turns off just the two new warnings. In other words, 
> if I want to adopt a new clang and I had `-Wimplicit-float-conversion` then 
> the only options I have are: fix all the new warnings (which might take time 
> given how much code I have), or turn off *all* of 
> `-Wimplicit-float-conversion`. I think it makes sense to force fixing all 
> warnings for the warning that's always a problem 
> `warn_impcast_integer_float_precision_constant`, but for 
> `warn_impcast_integer_float_precision` it would be nice if there were a 
> `-Wno-*` which disables only `warn_impcast_integer_float_precision` and 
> nothing else.
>  This would make it way easier to roll out a new clang.


Yes, you are right. I am thinking about making a new flag for the warning but 
make "definitely lose" one default on. How about 
`-Wimplicit-int-float-conversion`?


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 211381.
ziangwan added a comment.

Update diff.

1. Make "definitely lose" warning on by default.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,55 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy loss. 
+  if (SourceBT && TargetBT && SourceBT->isIntegerType() &&
+  TargetBT->isFloatingType()) {
+// Determine the number of precision bits in the source integer 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3279
+  "implicit conversion from %2 to %3 changes value from %0 to %1">,
+  InGroup, DefaultIgnore;
+

xbolva00 wrote:
> Drop ‘DefaultIgnore‘ here
I wish both warnings to be off by default and can be turned on by 
`-Wconversion` (`-Wimplicit-float-conversion`).


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1597627 , @jfb wrote:

> In D64666#1597193 , @aaron.ballman 
> wrote:
>
> > In D64666#1596660 , @xbolva00 
> > wrote:
> >
> > > I think we should warn in that case even if GCC does not warn.
> >
> >
> > Strong +1.
>
>
> Sorry if the phrasing was misleading: if we know for a fact that there's a 
> problem, we should warn unconditionally. If we don't know for a fact then the 
> warning should *not* be enabled by `-Wall` nor `-Wextra`. I don't really care 
> what GCC does by default, LLVM doesn't have to match every single thing. That 
> being said, if LLVM behaves differently then maybe the flag name should be 
> different.
>
> > In D64666#1596853 , @ziangwan 
> > wrote:
> > 
> >> Final review ping.
> > 
> > 
> > Please be sure to give reviewers enough time to respond to comments before 
> > pinging a review.
>
> Indeed. You haven't answered my first comment, I'd expect you to do so and 
> not "final ping" anything. I'm not saying you must do what I say, just that 
> you must answer comments, not ignore them.


Sorry to miss out your first comment, but I did answer it under @xbolva00's 
comment. I will make sure I put my response under the original one next time.

In D64666#1596655 , @ziangwan wrote:

> In D64666#1596512 , @xbolva00 wrote:
>
> > @jfb’s comment is not addressed yet.
> >
> > In D64666#1583629 , @jfb wrote:
> >
> > > I think you want to default-ignore the "may lose precision" warnings, but 
> > > not the ones that you know always lose precision.
> >
>
>
> In GCC, the int type -> float type conversion warning is disabled by default. 
> When the user uses `-Wconversion` or `-Wimplicit-float-conversion`, both 
> "definitely lose" warning and "may lose" warning are issued. The current 
> patch works the same way as GCC.


I think we definitely should issue the warning that says "may lose" precision. 
The reason is that a "may lose" warning encourages developers to examine their 
code and definitely helps them capture potential precision loss bugs. Also, in 
most case, we won't be able to know the exact value of the integer type, e.g. 
variables. If the developers are certain they are going to do an int->float 
conversion, they can always write explicit conversion, and the "may lose" 
warnings will go away.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

Final review ping.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 211245.
ziangwan added a comment.

Update diff

1. Fix trailing whitespaces.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,55 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy loss. 
+  if (SourceBT && TargetBT && SourceBT->isIntegerType() &&
+  TargetBT->isFloatingType()) {
+// Determine the number of precision bits in the source integer type.
+IntRange 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1596512 , @xbolva00 wrote:

> @jfb’s comment is not addressed yet.
>
> In D64666#1583629 , @jfb wrote:
>
> > I think you want to default-ignore the "may lose precision" warnings, but 
> > not the ones that you know always lose precision.
>


In GCC, the int type -> float type conversion warning is disabled by default. 
When the user uses `-Wconversion` or `-Wimplicit-float-conversion`, both 
"definitely lose" warning and "may lose" warning are issued. The current patch 
works the same way as GCC.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

Ping. I am going to submit this patch if nobody objects.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked an inline comment as done.
ziangwan added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:11429
+  S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
+TargetFloatValue.convertFromAPInt(SourceInt,
+  SourceBT->isSignedInteger(), llvm::APFloat::rmNearestTiesToEven);

scanon wrote:
> Why don't we just check that the result of the first conversion is opOK? I 
> don't think doing a round-trip check is required here.
I have changed the code to check the status of the first conversion only. 
Please review it again.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 209928.
ziangwan marked an inline comment as done.
ziangwan edited the summary of this revision.
ziangwan added a comment.

Update diff.

1. Adopt


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,55 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy loss. 
+  if (SourceBT && TargetBT && SourceBT->isIntegerType() &&
+

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 9 inline comments as done.
ziangwan added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:11430
+SmallString<32> PrettyTargetValue;
+TargetFloatValue.toString(PrettyTargetValue,
+  TargetPrecision);

nickdesaulniers wrote:
> xbolva00 wrote:
> > Can you check my older patch + tests + discussion? 
> > 
> > I had to use other way to get this string..
> > 
> > 
> And I don't think there's a test for this case? Or at least one that checks 
> the printed value?
I have checked your older patch.

IIUC, you take the log of number of precision bits and print the floating point 
value out. E.g. 2555+E9. I am doing it differently. I print out all the 
precision bits. E.g. 2555677.000. The reason is that I think it is more clear 
to print out `changes value from  to 1312` than `changes 
value from  to 1+e3`.



Comment at: clang/lib/Sema/SemaChecking.cpp:11430
+SmallString<32> PrettyTargetValue;
+TargetFloatValue.toString(PrettyTargetValue,
+  TargetPrecision);

ziangwan wrote:
> nickdesaulniers wrote:
> > xbolva00 wrote:
> > > Can you check my older patch + tests + discussion? 
> > > 
> > > I had to use other way to get this string..
> > > 
> > > 
> > And I don't think there's a test for this case? Or at least one that checks 
> > the printed value?
> I have checked your older patch.
> 
> IIUC, you take the log of number of precision bits and print the floating 
> point value out. E.g. 2555+E9. I am doing it differently. I print out all the 
> precision bits. E.g. 2555677.000. The reason is that I think it is more clear 
> to print out `changes value from  to 1312` than `changes 
> value from  to 1+e3`.
Test cases are added


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 209710.
ziangwan added a comment.

Update diff:

1. fix the spelling issue `may loses integer precision` -> `may lose precision`
2. issue more accurate warnings when `int->float` conversion involves constant 
integer.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+  float  = UL; // expected-warning {{implicit conversion from 'unsigned long' to 'float' changes value from  to 1312}}
+
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+}
+
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value from  to 1312}}
+
+  float g =  + ;
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value from 4445 to }}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may lose precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may lose precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may lose precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may lose precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may lose precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,61 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the integer type. Issue warnings for that accuracy 

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1583792 , @xbolva00 wrote:

> In D64666#1583667 , @ziangwan wrote:
>
> > In D64666#1583633 , @xbolva00 
> > wrote:
> >
> > > You can check also https://reviews.llvm.org/D52835. I hit there issue 
> > > which I didn't know how to solve.
> >
> >
> > Hi David,
> >
> > Can you elaborate what issues you encountered? Thank you.
>
>
> I had duplicated warning for C++11+ - my new warning and C++11’s narrowing 
> warning.
>
> Did you run ‘ninja check-clang’?


I ran `ninja check-clang` and got all passed.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 2 inline comments as done.
ziangwan added inline comments.



Comment at: clang/test/Sema/implicit-float-conversion.c:29
+  double e = 0.0;
+  double f = i + e;
+}

etalvala wrote:
> should this also test the case of:
> `
> long g = L;
> long h = g + a;
> `
> or is that unlikely to fail if the assignment-to-float variant succeeds?
> 
I feel like your case is covered in the testReturn() function.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan marked 6 inline comments as done.
ziangwan added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3245
+def warn_impcast_integer_float_precision : Warning<
+  "implicit conversion from %0 to %1 may loses integer precision">,
+  InGroup, DefaultIgnore;

nickdesaulniers wrote:
> `may lose` or just `loses`.
"may lose". Whether the precision loss really happens depends on the value of i 
(how many precision bits i has).

For example, `float e = i` where i is an integer. If i is 15 then there isn't 
precision loss. If i is 2, then there is precision loss.



Comment at: clang/lib/Sema/SemaChecking.cpp:11417
+llvm::APFloat SourceToFloatValue(
+  S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)));
+

nickdesaulniers wrote:
> should this use `SourceBT` rather than `TargetBT`?
Should be `TargetBT`. I've updated the naming convention to make it more clear.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 209611.
ziangwan added a comment.

Fix spelling / comment / naming issues.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+}
+
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value}}
+  
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+}
+
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value}}
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value}}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may loses integer precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may loses integer precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may loses integer precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may loses integer precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may loses integer precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may loses integer precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may loses integer precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may loses integer precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,49 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower significand
+  // than the floating point type. Issue warnings for that accuracy loss. 
+  if (SourceBT && TargetBT &&
+  SourceBT->isIntegerType() && TargetBT->isFloatingType()) {
+// Determine the number of precision bits in the source integer type.
+IntRange SourceRange = GetExprRange(S.Context, E, S.isConstantEvaluated());
+unsigned int SourcePrecision = SourceRange.Width;
+
+// Determine the number of 

[PATCH] D64666: Allow Clang -Wconversion, -Wimplicit-float-conversion warns about integer type -> floating point type implicit conversion precision loss.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

In D64666#1583633 , @xbolva00 wrote:

> You can check also https://reviews.llvm.org/D52835. I hit there issue which I 
> didn't know how to solve.


Hi David,

I have read your patch. I think that integer-to-floating-point conversion does 
not necessarily leads to precision loss. We need to have extra condition about 
whether to issue warnings.

For example,

1. Convert L to float leads to precision loss. The reason is that 
L needs 48 bits of precision where float only has 24 bits of 
precision.
2. Convert 22 to float does not lead to precision loss. 22 needs 18 
bits of precision only.


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

https://reviews.llvm.org/D64666



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

[PATCH] D64666: Allow Clang -Wconversion, -Wimplicit-float-conversion warns about integer type -> floating point type implicit conversion precision loss.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan updated this revision to Diff 209591.
ziangwan added a comment.

Removed patch.patch.


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

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c

Index: clang/test/Sema/implicit-float-conversion.c
===
--- /dev/null
+++ clang/test/Sema/implicit-float-conversion.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -verify -Wno-conversion -Wimplicit-float-conversion
+
+
+long testReturn(long a, float b) {
+  return a + b; // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+}
+
+
+void testAssignment() {
+  float f = 22;
+  double b = L;
+  
+  float ff = L; // expected-warning {{implicit conversion from 'long' to 'float' changes value}}
+  
+  long l = L;
+  float fff = l; // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+}
+
+
+void testExpression() {
+  float a = 0.0f;
+  float b = L + a; // expected-warning {{implicit conversion from 'long' to 'float' changes value}}
+  float c =  + 2223; // expected-warning {{implicit conversion from 'int' to 'float' changes value}}
+
+  int i = 0;
+  float d = i + a; // expected-warning {{implicit conversion from 'int' to 'float' may loses integer precision}}
+  
+  double e = 0.0;
+  double f = i + e;
+}
Index: clang/test/Sema/ext_vector_casts.c
===
--- clang/test/Sema/ext_vector_casts.c
+++ clang/test/Sema/ext_vector_casts.c
@@ -115,12 +115,12 @@
   vl = vl + t; // expected-warning {{implicit conversion loses integer precision}}
   
   vf = 1 + vf;
-  vf = l + vf;
+  vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may loses integer precision}}
   vf = 2.0 + vf;
   vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
-  vf = vf + 0x;
+  vf = vf + 0x; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value}}
   vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
   
-  vd = l + vd;
-  vd = vd + t;
+  vd = l + vd; // expected-warning {{implicit conversion from 'long' to 'double2' (vector of 2 'double' values) may loses integer precision}}
+  vd = vd + t; // expected-warning {{implicit conversion from '__uint128_t' (aka 'unsigned __int128') to 'double2' (vector of 2 'double' values) may loses integer precision}}
 }
Index: clang/test/Sema/conversion.c
===
--- clang/test/Sema/conversion.c
+++ clang/test/Sema/conversion.c
@@ -233,7 +233,7 @@
   takes_int(v);
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'int' to 'float' may loses integer precision}}
   takes_double(v);
   takes_longdouble(v);
 }
@@ -244,8 +244,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long' to 'float' may loses integer precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long' to 'double' may loses integer precision}}
   takes_longdouble(v);
 }
 
@@ -255,8 +255,8 @@
   takes_int(v); // expected-warning {{implicit conversion loses integer precision}}
   takes_long(v);
   takes_longlong(v);
-  takes_float(v);
-  takes_double(v);
+  takes_float(v); // expected-warning {{implicit conversion from 'long long' to 'float' may loses integer precision}}
+  takes_double(v); // expected-warning {{implicit conversion from 'long long' to 'double' may loses integer precision}}
   takes_longdouble(v);
 }
 
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -11400,6 +11400,54 @@
 }
   }
 
+  // If we are casting an integer type to a floating point type, we might
+  // lose accuracy if the floating point type has a narrower signicand
+  // than the floating point type. Issue warnings for that accuracy loss. 
+  if (SourceBT && TargetBT &&
+  SourceBT->isIntegerType() && TargetBT->isFloatingType()) {
+// Determine the number of precision bits in the source integer type.
+IntRange SourceRange = GetExprRange(S.Context, E, S.isConstantEvaluated());
+unsigned int SourcePrecision = SourceRange.Width;
+
+// Determine the number of precision bits in the 

[PATCH] D64666: Allow Clang -Wconversion, -Wimplicit-float-conversion warns about integer type -> floating point type implicit conversion precision loss.

[Ziang Wan via Phabricator via cfe-commits]

ziangwan created this revision.
ziangwan added reviewers: kongyi, pirama, rsmith, nickdesaulniers.
Herald added subscribers: cfe-commits, jdoerfert, jfb, mgrang, javed.absar.
Herald added a project: clang.

These code:

  long i = L;
  float a = L:
  float b = a + i;

Will now issue warnings:

  line 2: implicit conversion from 'long' to 'float' changes value from 
 to 1312 [-Wimplicit-float-conversion]
  line 3: implicit conversion from 'long' to 'float' may loses integer 
precision. [-Wimplicit-float-conversion]

The same feature is present in GCC but not currently in clang.


Repository:
  rC Clang

https://reviews.llvm.org/D64666

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/conversion.c
  clang/test/Sema/ext_vector_casts.c
  clang/test/Sema/implicit-float-conversion.c
  patch.patch



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

[PATCH] D63822: [Driver] Fix style issues of --print-supported-cpus

[Ziang Wan via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D63822



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

[PATCH] D63822: [Driver] Delete --print-supported-cpus in favor of -mcpu=? or -mtune=?

[Ziang Wan via Phabricator via cfe-commits]

ziangwan added a comment.

Hey @MaskRay , can you explain why we should remove --print-supported-cpus. 
There already are similar options in clang such as `--print-effective-triple` 
and `--print-libgcc-file-name`. On the other hand, I almost never see an option 
goes like `-xxx=?`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63822



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