auto-indent research result and proposal

Sat Jan 18 09:33:35 UTC 2014

On 18/01/14 13:35, Dirk Hohndel wrote:
> On Sat, 2014-01-18 at 11:28 -0200, Danilo Cesar Lemes de Paula wrote:
>> So, I've been checking ways to auto indent code before commiting and I
>> used subsurface as a guinea pig for my tests with clang-format.
>>
>> The approach I took is the same that gstreamer uses, using a git hook to
>> check all files that are being commited and run clang-format against them.
>> This means that all checked files will have to follow the same rules
>> (although we can tweak the hook to add exceptions). So, before a commit,
>> your whole file needs to pass the rules, otherwise you'll see a warning
>> (and this warning remains until the file is fixed by someone)
> 
> You see a warning or the commit isn't run?

Right now the commit won't run. But we can change that to show a nice
"your commit don't follow our guidelines, continue anyway?".

>> Basically, clang-format needs a .clang-format file in the root folder,
>> containing the rules. I started a rules file [1] that I think would make
>> sense (following Dirk's comments and a few more patters I saw people using).
>> I'm also sending a diff file [2] as a preview of what it can do.
>>
>> I enjoyed the final result and I'm probably going to use in other projects.
>> But it's not perfect, I have a list of flaws:
>>
>> 1 - header files with classes: is common to use public:/private: at the
>> begging of the line. clang indents it with a tab.
> 
> Yuck - that's really ugly.

fixed with AccessModifierOffset: -8

>> 2 - if(something) {} will be replaced by if (something) {
>>     } - it's cool. However, it can't recognize Qt controls statements
>> (Q_FOREACH) and assumes it's a function. so it will remove the space and
>> put a line break before the brace as in:
>>     q_foreach(something, something)
>>     {
>>       ....
>>     }
> 
> Not cool

I can't do much about it.

> 
>> 3 - it fixes all the arithmetics operators with a space and uses the
>> form QString* str instead of QString *str everywhere. However, inside
>> the non recognized control structures (q_foreach again) it thinks that
>> it's an arithmetic operator and expects the following usage:
>>     q_foreach(QString * str, list)... not cool as foreach(QString* str)
> 
> I really don't like either.
> 
> type *variable;

Not sure if we're talking the same thing here, but "PointerBindsToType:
false" outputs "type *variable;" (but still not inside the foreach macro)
> 
> 
>> 4 - it uses and expect tabstop=8 (kernel's guide expects the same), but
>> escaped new lines will look unindented if you use tabstop!=8.
> 
> I don't understand that.

not an issue if you use tabstop=8 (I'm talking about vim here, I don't
know much about how other editors deals with tab). the following
...somthing........\
...somethingbigger.\
other..............\

might look ugly if the editor doesn’t consider tab as 8 stops.

> 
>> 5 - I couldn't find a way to put space after a cast, so casts will looks
>> like:
>>     (struct dive*)index.data(); instead of (struct dive*) index.data(),
>> which would be easier to see. IHMO.
> 
> That one I don't care as much about
> 
>> 6 - I never used git-hook before but I don't think it can be included in
>> the git repository. What gstreamer does, as example, is to install the
>> hook during the ./configure phase. I think we could do the same with qmake.
> 
> I haven't looked into this myself, but given that this would only be
> needed for developers I'm sure we can figure out how to install
> things...
> 
>> Even with those issues, I think it's a low price to be paid for
>> coding-style consistency in the whole code make the developer's life easier.
> 
> Not sure. If every instance of public/private in a .h files creates a
> warning and the Qt control structures aren't recognized... that's a non
> starter for me.
> Can the warnings be post-processed or otherwise filtered.

I managed to fix the public/private handlers, but I'm not sure how to
solve the controls structures.

>> Again, it's only a proposal and it's understandably if it doesn't fit
>> subsurface's needs.
> 
> I think it may be a good start, but as I said, without some ability to
> fix a couple of the glaring issues I think it won't solve the issue I'm
> trying to solve...
> 
>> But if you guys are ok with it and want to give it a try, I can prepare
>> a proper committ
> 
> I think I would like to play with it a little and if we can address the
> issues, then yes, I'd love it.

If you want to try: save .clang-format in the root folder, then use
clang-format-3.5 -i -style=file qt-ui/*.cpp (or .c or .h) to see the
diffs. You can check the available options in
http://clang.llvm.org/docs/ClangFormatStyleOptions.html

I've been reading the man page, it might be possible to check only the
diff lines before committing a patch (using the broken-in-debian
clang-format-diff or using -lines as argument). That way, you can ignore
the warning before the commit and never hear about that warning again
(until you change that same place of code again).

> Thanks for your work on this!
> 
> /D
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20140118/88efcde3/attachment-0001.sig>