[PATCH] D16310: new clang-tidy checker misc-long-cast

Richard via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 22 09:25:45 PST 2016


LegalizeAdulthood added a comment.

I find this an interesting discussion.  I do not mean to imply that my remarks constitute any sort of demand that this check produce my suggestion for a fixit.

In http://reviews.llvm.org/D16310#333416, @danielmarjamaki wrote:

> I expect that this warning will in most cases be fixed by moving the cast.


For a seasoned C++ developer having authored the offending code, I agree.  However, I have seen lots of code where things were done that were just unnecessary.

> I believe there is often a bug in such code.


I think it depends on who was writing it.

> If you want that we hide these bugs by removing the casts I can do it.. but I personally think that is wrong.


I'm simply saying that **if** it is going to suggest a fixit, then the fixit suggested should be one that doesn't change the meaning of the code as written.

I agree that such a suggested fixit would not be the correct change in all cases, since clang-tidy can't know the original intent of the author.

However, suppose we had code like this:

  long f(int a) {
    return a*1000;
  }

This code is not going to issue any warnings about potential loss of precision, but by simply adding the cast on the result expression, suddenly I'm getting warnings about possible loss of precision.

In both cases, the precision loss is a potentiality.

The check assumes that placing the cast there is an indicator of a bug.

When I look at such code with a redundant cast, I would infer the opposite: the programmer is adding stuff that isn't necessary.

Maybe our differing attitude on what the cast implies is a reflection of the difference in our experiences in working on different code bases.  Am I just so unlucky as to have been exposed to so much **badly written** C++ over the years?  The checks that I have added and would like to continue to add are primarily focused on "cleaning up the mess" that I've seen in so many C++ code bases over the years.  I have seen many uses of unnecessary casting over the years, as opposed to misplaced casts that were intended to prevent loss of precision.

Without access to the original intent (locked up in someone's brain somewhere), whether or not a loss of precision is a problem can only be determined by dynamic analysis or some other form of runtime testing.

Perhaps because both of our interpretations are valid but different points of view, it is best that this check not offer any fixit at all.  If it does offer a fixit, then there should be a configuration option to say which kind of fixit to prefer.  A developer can then use a workflow like this:

1. Run the tool with no fixits to see if there are any potential problems.
2. Examine each instance and decide if the cast is redundant or misplaced.
3. For redundant casts, run the tool again configured to drop the redundant cast and selectively apply the fixits on the appropriate instances.
4. For misplaced casts, run the tool again configured to move the cast and selectively apply the fixits on the appropriate instances.

This leaves the tool operating conservatively by default, yet still providing the developer with the means of deciding the intent on each flagged instance and applying the appropriate fix for each case.


Repository:
  rL LLVM

http://reviews.llvm.org/D16310





More information about the cfe-commits mailing list