On Mon, Mar 23, 2015 at 11:49 PM, Daniel Jasper <[email protected]> wrote:
> I am actually not sure how clang-tidy should handle cases like this (where > two checks would fix the same code in different ways) and it's probably > something we should figure out in the long run. > > In this case, the else after return (yes, we do have a check for that > although it doesn't yet offer automatic fixing) should take precedence as > there is an explicit rule in the coding standards. > > Of course, that still leaves the question of whether clang-tidy should > also simplify: > > if (a) > return true; > return false; > > to > > return a; > Yep - if/when the check gets to catching this (which it seems it should - there's nothing fundamentally 'better' about this code than the other code) then John's issue's apply again and I'd be inclined to agree with him - though the exact nature of the suppression, I'm not sure - possibly suppress the warning if the 'if' is an else and the prior if returns on all paths? Dunno. > > On Tue, Mar 24, 2015 at 7:41 AM, David Blaikie <[email protected]> wrote: > >> >> >> On Mon, Mar 23, 2015 at 11:30 PM, Daniel Jasper <[email protected]> >> wrote: >> >>> I don't feel strongly about this, and I can see some of your reasoning. >>> However, an "if (a) return true; else return false;" is very suspect to me >>> and I think "return a;" is more readable, independent of whether it is at >>> the end of a chain or not. >>> >>> However, there is an underlying issue here. The existing code is >>> violating LLVM's coding standard of "don't use else after return" ( >>> http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return). >>> So in these chains, all of the "else"s should be removed. >>> >> >> & that incidentally (sounds like more by accident than design) suppresses >> the warning. Though, arguably, this clang-tidy check perhaps shouldn't be >> passing judgment on that particular case and we should have an >> else-after-return check (we probably already do) for that explicitly. >> >> That said, I don't much mind this check happening to provide >> else-after-return catching in this way. (which is to say, in a round about >> way, I agree with you Daniel) Maybe it's just a case of two different style >> checkers happening to catch the same code for different reasons, and the >> same potential code change addresses both of them. >> >> >>> >>> On Tue, Mar 24, 2015 at 7:17 AM, Richard <[email protected]> wrote: >>> >>>> Thanks for your ideas, John, I'm going to incorporate this into the >>>> `clang-tidy` check. >>>> >>>> >>>> http://reviews.llvm.org/D8532 >>>> >>>> EMAIL PREFERENCES >>>> http://reviews.llvm.org/settings/panel/emailpreferences/ >>>> >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
