[PATCH] D71686: Fix false positive in magic number checker

Sun Dec 22 09:35:05 PST 2019

0x8000-0000 marked 2 inline comments as done.
0x8000-0000 added a comment.

In D71686#1794366 <https://reviews.llvm.org/D71686#1794366>, @aaron.ballman wrote:

> In D71686#1794360 <https://reviews.llvm.org/D71686#1794360>, @0x8000-0000 wrote:
>
> > In D71686#1794330 <https://reviews.llvm.org/D71686#1794330>, @aaron.ballman wrote:
> >
> > > In D71686#1794053 <https://reviews.llvm.org/D71686#1794053>, @0x8000-0000 wrote:
> > >
> > > > My take: this change fixes a user-reported bug, and does not cause any known regressions. I think we should integrate this.
> > >
> > >
> > > I sort of wonder whether we want to document this as a blessed approach to silencing the warning. I'm not certain if it's too obtuse or not, but I notice the check has no documented ways to silence the diagnostic aside from using the correct kind of magic number or adding it to a list of excluded magic numbers.
> >
> >
> > You mean Hyrum's Law <https://www.hyrumslaw.com/> is not sufficient?
> >
> > The check can be silenced with the regular NOLINT, or with defining and using a constant/enum. Using this "backdoor" way seems even more cumbersome and confusing than the NOLINT. At least with NOLINT it is clear what you're doing, and somebody else can grep for it and fix it if it is appropriate.
>
>
> My concern is that `NOLINT` is insufficient. Consider: `foo(12, 42, 18);` where the `42` is not desired to be warned about due to domain-specific knowledge but the `12` and `18` are.
>
> However, I am not convinced that you're wrong either -- casting is cumbersome. But it's also a somewhat well-used workaround to silence warnings (casting to void silencing unused value warnings being a common example).
>
> For right now, we can leave it as a bug -- we can decide to bless the approach later.

Fair enough; I'm aiming for good precision and recall by default.

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

https://reviews.llvm.org/D71686