[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-12-19 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment. I posted to cfe-dev: http://lists.llvm.org/pipermail/cfe-dev/2017-December/056450.html Repository: rL LLVM https://reviews.llvm.org/D39462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-12-19 Thread Brian Cain via Phabricator via cfe-commits
bcain added a comment. In https://reviews.llvm.org/D39462#959822, @phosek wrote: > FWIW we've already rolled Clang that contains > `-Wtautological-constant-compare` to our codebase and we had to set > `-Wno-tautological-constant-compare` globally because we were getting bogus > warnings in man

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-12-19 Thread Petr Hosek via Phabricator via cfe-commits
phosek added a comment. FWIW we've already rolled Clang that contains `-Wtautological-constant-compare` to our codebase and we had to set `-Wno-tautological-constant-compare` globally because we were getting bogus warnings in many places, similarly to https://reviews.llvm.org/D41368. If that wa

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-27 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D39462#936566, @bcain wrote: > I'd like to understand/resurrect this change, so I'll try to summarize. > Please correct this as appropriate: > > 1. We got here because libc++ has code that triggers a warning for some > targets (those whos

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-27 Thread Brian Cain via Phabricator via cfe-commits
bcain added a comment. I'd like to understand/resurrect this change, so I'll try to summarize. Please correct this as appropriate: 1. We got here because libc++ has code that triggers a warning for some targets (those whose `int` and `long` have the same size). 2. This change would "move" the

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That's the template type-propagation problem: the template was invoked with a size_t, but that's erased down to the canonical type by the template machinery. That can't be fixed within the template, but at use sites, we could theoretically recognize that e.g. the resu

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D39462#922847, @rjmccall wrote: > In https://reviews.llvm.org/D39462#922844, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D39462#922826, @rjmccall wrote: > > > > > I don't speak for the entire project, but I'm not sure I'm interested

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I see. The problem now, though, is that things involving, say, a size_t and a numeric_limits will never warn. Repository: rL LLVM https://reviews.llvm.org/D39462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D39462#922826, @rjmccall wrote: > I don't speak for the entire project, but I'm not sure I'm interested in the > diagnostic you're actually offering to contribute here. It may produce a > warning on your specific test case, but I think it

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment. In https://reviews.llvm.org/D39462#922847, @rjmccall wrote: > In https://reviews.llvm.org/D39462#922844, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D39462#922826, @rjmccall wrote: > > > > > I don't speak for the entire project, but I'm not sure I'm interested in

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives.

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D39462#922844, @lebedev.ri wrote: > In https://reviews.llvm.org/D39462#922826, @rjmccall wrote: > > > I don't speak for the entire project, but I'm not sure I'm interested in > > the diagnostic you're actually offering to contribute here. It

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-11 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D39462#917421, @rjmccall wrote: > So, that change makes this very interesting, because I think the right way of > looking at it is as the first in a larger family of warnings that attempt to > treat typedefs as if they were a much stronger

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. So, that change makes this very interesting, because I think the right way of looking at it is as the first in a larger family of warnings that attempt to treat typedefs as if they were a much stronger type-system feature, i.e. that warn about all sorts of conversions

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-11-06 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri updated this revision to Diff 121743. lebedev.ri edited the summary of this revision. lebedev.ri added a comment. In https://reviews.llvm.org/D39462#912011, @rjmccall wrote: > The actual choice of integer type is not stable across targets any more than > the size is. In practice, peo

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-10-31 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. > The internal canonical types are compared, so all this typedef sugar will be > silently ignored. Yes, and that's precisely the problem. On many 32-bit platforms, both "size_t" and "uint32_t" are typedefs for "unsigned int"; on some 32-bit platforms, "size_t" is an

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-10-31 Thread Shoaib Meenai via Phabricator via cfe-commits
smeenai added a comment. This solves the issue in https://reviews.llvm.org/D39149, at least. Repository: rL LLVM https://reviews.llvm.org/D39462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-10-31 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri updated this revision to Diff 121044. lebedev.ri added a comment. In https://reviews.llvm.org/D39462#912011, @rjmccall wrote: > The actual choice of integer type is not stable across targets any more than > the size is. In practice, people often don't use int and long, they use > st

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-10-31 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The actual choice of integer type is not stable across targets any more than the size is. In practice, people often don't use int and long, they use standard typedefs like size_t and uint64_t, but the actual type chosen for size_t is arbitrary when there are multiple

Fwd: [PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

2017-10-31 Thread Roman Lebedev via cfe-commits
: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent To: lebedev...@gmail.com, rich...@metafoo.co.uk, rjmcc...@gmail.com, dblai...@gmail.com Cc: mclow.li...@gmail.com, aaron.ball...@gmail.com, bc...@codeaurora.org, smee...@fb.com, jkor...@apple.com