Just as an example, I picked totally at random, one c++ program to run the google code style checker. There are clearly some valid points it found. I think it would good to start to adapt this tool or write a new tool to do style checking and to start to better formalize the llvm rules. I ran it against Target/Target.cpp Target.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Target.cpp:10: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4] Target.cpp:22: Found C++ system header after other header. Should be: Target.h, c system, c++ system, other. [build/include_order] [4] Target.cpp:24: Do not use namespace using-directives. Use using-declarations instead. [build/namespaces] [5] Target.cpp:26: Is this a non-const reference? If so, make const or use a pointer. [runtime/references] [2] Target.cpp:65: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Target.cpp:69: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Target.cpp:73: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Target.cpp:95: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Target.cpp:100: Lines should be <= 80 characters long [whitespace/line_length] [2] Target.cpp:100: Use int16/int64/etc, rather than the C type long [runtime/int] [4] Target.cpp:49: Add #include <string> for string [build/include_what_you_use] [4] Done processing Target.cpp Total errors found: 12
On 05.06.2012 02:56, reed kotler wrote:> Just as an example, I picked totally at random, one c++ program to run > the google code style checker. > > There are clearly some valid points it found. I think it would good to > start to adapt this tool > or write a new tool to do style checking and to start to better > formalize the llvm rules. > > I ran it against Target/Target.cpp > > Target.cpp:10: Line ends in whitespace. Consider deleting these extra > spaces. [whitespace/end_of_line] [4]We have an incredible amount of those, and fixing them would create far too much churn. I generally fix them on the code I touch.> Target.cpp:22: Found C++ system header after other header. Should be: > Target.h, c system, c++ system, other. [build/include_order] [4]The LLVM convention is to reverse the "usual" order. The first file any .cpp includes should be its own .h. Then other project headers. Then, in the case of Clang, the LLVM headers. And finally system and standard headers.> Target.cpp:24: Do not use namespace using-directives. Use > using-declarations instead. [build/namespaces] [5]LLVM convention is to use a using directive for the project namespace.> Target.cpp:26: Is this a non-const reference? If so, make const or use > a pointer. [runtime/references] [2]What? Why? Sebastian
Did you agree with the comment about the use of long long from the tool? Anyway, it's not really important to me that we adopt any specific google code rules over the current llvm rule. The point is to that Google has a deeper set of conventions and it would be a good starting point for us. Also, they have a tool which checks a lot of this. The lack of a tool for llvm style check is what originally caught my attention in this area. I don't like doing work that a robot can do for me. The Google tool has a wish list of other things that I would not be opposed to personally adding should we go that way. The point is to collect the kinds of style errors that send a patch back to the author and to try and implement those in the style checker. I would even open up a bug against the style checker for each such thing that is not checked. Not wanting to clean up the white space is exactly a simple but good example of technical debt that we are incurring. In that case it's very simple to see. We have a rule about that for our style and because we are too busy with other things, then we have allowed the technical debt to rise to a point where we don't want to ever pay it, or so you say. When I ran a script over the Target subtree, my simple check showed 20,000 violations of tab, line length and spaces at the end of line rules. On 06/05/2012 04:50 AM, Sebastian Redl wrote:> On 05.06.2012 02:56, reed kotler wrote: >> Just as an example, I picked totally at random, one c++ program to run >> the google code style checker. >> >> There are clearly some valid points it found. I think it would good to >> start to adapt this tool >> or write a new tool to do style checking and to start to better >> formalize the llvm rules. >> >> I ran it against Target/Target.cpp >> >> Target.cpp:10: Line ends in whitespace. Consider deleting these extra >> spaces. [whitespace/end_of_line] [4] > We have an incredible amount of those, and fixing them would create far > too much churn. I generally fix them on the code I touch. >> Target.cpp:22: Found C++ system header after other header. Should be: >> Target.h, c system, c++ system, other. [build/include_order] [4] > The LLVM convention is to reverse the "usual" order. The first file any > .cpp includes should be its own .h. Then other project headers. Then, in > the case of Clang, the LLVM headers. And finally system and standard > headers. >> Target.cpp:24: Do not use namespace using-directives. Use >> using-declarations instead. [build/namespaces] [5] > LLVM convention is to use a using directive for the project namespace. >> Target.cpp:26: Is this a non-const reference? If so, make const or use >> a pointer. [runtime/references] [2] > What? Why? > > Sebastian > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Tue, Jun 5, 2012 at 4:50 AM, Sebastian Redl <sebastian.redl at getdesigned.at> wrote:> On 05.06.2012 02:56, reed kotler wrote: >> Just as an example, I picked totally at random, one c++ program to run >> the google code style checker. >> >> There are clearly some valid points it found. I think it would good to >> start to adapt this tool >> or write a new tool to do style checking and to start to better >> formalize the llvm rules. >> >> I ran it against Target/Target.cpp >> >> Target.cpp:10: Line ends in whitespace. Consider deleting these extra >> spaces. [whitespace/end_of_line] [4] > We have an incredible amount of those, and fixing them would create far > too much churn. I generally fix them on the code I touch.That's been the prevailing opinion, except it doesn't seem to hold - both SubVersion and Git have the ability to annotate/diff with whitespace ignorance, so adding/removing whitespace shouldn't affect your source control experience (ok, I guess ViewVC might not have such smarts) - unless a whitespace run is entirely removed (which isn't relevant to trailing whitespace removal, since there's always the newline there - and 80 col fixes are generally appreciated by Chris at least, even if they're not in code being touched by the developer - not sure how he feels about a blanket 80 col fix, though I'd be more inclined to do that if we had a tool to ensure no regressions) That said, if we could get tools in place that could break the build when (and before) we submit new violations, that's probably sufficient - no CR headaches and no big churn. It's just that most such tools aren't smart enough to just do diff based validation, so then you end up arguing in favor of fixing the whole codebase before you add validators.>> Target.cpp:22: Found C++ system header after other header. Should be: >> Target.h, c system, c++ system, other. [build/include_order] [4] > The LLVM convention is to reverse the "usual" order. The first file any > .cpp includes should be its own .h. Then other project headers. Then, in > the case of Clang, the LLVM headers. And finally system and standard > headers. >> Target.cpp:24: Do not use namespace using-directives. Use >> using-declarations instead. [build/namespaces] [5] > LLVM convention is to use a using directive for the project namespace. >> Target.cpp:26: Is this a non-const reference? If so, make const or use >> a pointer. [runtime/references] [2] > What? Why?One of Google's style guidelines, also not relevant/used by LLVM - LLVM is just fine having non-const ref out or in/out parameters.> > Sebastian > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev