- User Since
- Aug 28 2015, 5:49 PM (362 w, 3 d)
Nov 6 2015
@alexfh: Ah, I forgot about this review. I will mark it as abandoned, because I have already started work on new check for localizing variables. It will have the wider scope that Eugene proposed originally, that is to move variable declarations closer to their first use. If you're curious how the code looks, you can see it on my Github fork: https://github.com/piotrdz/clang-tools-extra, as I said in discussion on cfe-dev some time ago. I will submit this new check in a new review once I finish it.
Oct 25 2015
@eugene: thanks for clarifying that and your advice. I made the commit and it seems I didn't break anything, yet :-).
I have obtained now commit rights but I want to make sure: I can commit this patch now that it is accepted? That is, accepting the review is the same as saying "LGTM", right?
Oct 24 2015
Thanks for accepting the review. However, I still have to ask someone to make the commit. Or should I obtain commit rights myself? If so, how do I do that?
Oct 21 2015
Any other comments?
Addressed last issues in review
Oct 20 2015
There you go, third revision. I think this version should be ready for commit.
Addressed latest review issues.
Oct 18 2015
I think this is a more complete version, and we're getting closer to getting my patch committed. @sbenza: Do you agree?
I fixed the issues found in review and added a few other enhancements:
- add config options to allow integer and pointer casts in conditionals, e.g. "if (pointer)",
- add support for NULL macro cast to boolean,
- ignore usage of variable declaration in control statements, e.g. "if (int x = function())",
- add support case of false literal to pointer conversion in pre-C++11 standards
- propose NULL instead of nullptr in replacements in pre-C++11 standards
Oct 15 2015
@sbenza: Thanks for the review. I have added my comments with expanations as necessary, and I have already fixed some issues you reported. I'm not posting a new diff at this time, as I want to finish with remaining issues, and then post updated diff once I'm ready with everything. I'll also work on adding a config option to conditionally allow for pointer to bool casts, as discussed on cfe-dev mailing list.
Oct 13 2015
Ah, no, I don't - so far I have relied on other people to help with commits. I'd appreciate it if you could commit this for me. Thanks.
@aaron.ballman: Thanks for the review. I uploaded an updated diff with fixes.
Addressed issues found in review
Oct 11 2015
Sep 9 2015
\o/ Yay! Thanks!
@alexfh: here we go again. Any comments?
Again, addressed all review issues. I hope this is the final version
Sep 8 2015
@alexfh: What do you think now? Are we getting nearer to making a commit?
I hope this is the final re-write of my code. In this version, I addressed most recent review comments, while also refactoring code to better handle template specializations and correctly display diagnostics in case when function definition is visible. In the end I had to change over half of the code, but perhaps it was worth it.
Sep 5 2015
Now that I fixed all review issues, I think this version would be acceptable for commit. @alexfh: do you agree?
There are of course two outstanding issues marked with TODO and FIXME comments, but these are areas of improvement. I would like to first discuss the best solution to them, before attempting to fix them in code.
I addressed all latest review issues.
Sep 2 2015
In this third version I did the following:
- fixed problems which I noticed with template specializations,
- changed output diagnostics to be more detailed, covering multiple declarations
- added FixIt hints to refactor inconsistent declarations to parameter names we see in definition
- added new template-related testcases and extended existing ones with checking of note diagnostics and FixIt hints
Aug 31 2015
Now this issue with templates is a bit difficult for me. I tried everything that seems to have made sense, but I still get output like this:
/work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: warning: function 'templateFunction' has other declaration with different parameter name [readability-inconsistent-declaration-parameter-name] void templateFunction(float b) ^ /work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:6: note: other declaration seen here void templateFunction(float b) ^ /work/clang-trunk/clang-tools-extra/test/clang-tidy/readability-inconsistent-declaration-parameter-name.cpp:47:29: note: parameter 1 is named 'b' here, but 'a' in other declaration void templateFunction(float b) ^
Applied fixes for most issues found in review.
@alexfh: Thanks for reviewing my code. The amount of comments makes it pretty clear that it's my first contribution here :-). But no worries, I'll address each issue.
Aug 30 2015
@eugene: I don't understand, what does declaring function with "void" argument have in common with this review? I only check here variable declarations inside functions.
I noticed a few things I should have done better:
- use std::set instead of std::unordered_set (it doesn't seem to be used elsewhere)
- move checking set of reported functions to just before emitting diagnostic
- improved documentation
- changed diff directory to inside clang-tools-extra repository