[Kde-pim] Review Request 119576: how to protect a command from a change by using astyle

[Dan Vrátil]

> On Aug. 4, 2014, 3:20 p.m., Dan Vrátil wrote:
> > Sorry, this is not the right solution - there are many other places in Akonadi where we use X-AKAPPEND that could potentially get broken by astyle and we can't just workaround everything by making it a string. Altering the code just so that astyle understands it is not the right approach :)
> > 
> > As I mentioned in my email to you, next time I just have to be more careful when reviewing the changes.
> 
> Kevin Krammer wrote:
>     Are you sure?
>     Here it is a string, just that the quotes are added by the macro.
>     
>     Do we have any code that uses a character that looks like a minus but isn't and is not inside a string literal?
> 
> Guy Maurel wrote:
>     NO, we don't have (akonadi) *many* places where we use X-AKAPPEND.
>     The first place is libs/protocol_p.h:84:#define AKONADI_CMD_ITEMCREATE       "X-AKAPPEND"
>     Many other places are such server/tests/unittest/akappendhandlertest.cpp:157:        return "C: 2 X-AKAPPEND "...
>     using it as a string.
>     Under kdepimlibs/akonadi you can find:
>     src/core/jobs/itemcreatejob.cpp:176:        command += " X-AKAPPEND ";
>     
>     Having a look to src/5/kde/kdepimlibs/akonadi
>     There are more than 7000 lines with a minus-sign.
>     All are arithmetic operators, or strings with "" or charcter with ''.
> 
> David Jarvie wrote:
>     Even if the existing code is changed so that astyle can be run without breaking it, there is always the potential for future changes which could be broken by astyle. So I would agree with Dan that altering the code is the wrong approach, since astyle can never be guaranteed to work without issues.
> 
> Dan Vrátil wrote:
>     That was pretty much my point, I just expressed it very poorly. Thanks David
> 
> Guy Maurel wrote:
>     I think it is not fair again astyle to write such a comment.
>     
>     Let us have a look to the gcc bug list: https://gcc.gnu.org/bugzilla/buglist.cgi?component=c%2B%2B&order=assigned_to%20DESC%2Cchangeddate%20DESC%2Cbug_status%2Cpriority%2Cbug_id&product=gcc&query_based_on=&query_format=advanced&resolution=---
>     You may see many dozens of bugs which are yet unassigned. Some of them are more than one year old!
>     If the user wants to go on, he/she must change the source code to get it compiled, even if the code is correct, until the bug is fixed.
>     Let me change the sentence above to:
>     "Even if the existing code is changed so that gcc can be run without breaking it, there is always the potential for future changes which could be broken by gcc."
>     Could you agree that?
>     I not.

Sorry, I think you are comparing apples and oranges. This is not a bug in astyle, it's a fault by design. Astyle is not a fullfeatured C++ parser, so it can never understand that X-AKAPPEND is a macro in this particular context and that it should be skipped. Cases when a compiler is not able to compile a valid code are rather rare and they tend to get fixed quickly, while this astyle problem is pretty much unfixable.

I thought that the point of these coding-style changes was to make the code easier to read and "prettier", which is subjective, I admit, but I don't think that your patch improves readability or makes the code look nicer.


- Dan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119576/#review63774
-----------------------------------------------------------


On Aug. 2, 2014, 7:03 p.m., Guy Maurel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119576/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2014, 7:03 p.m.)
> 
> 
> Review request for KDEPIM and Dan Vrátil.
> 
> 
> Repository: akonadi
> 
> 
> Description
> -------
> 
> astyle cannot (yet) distinguite a hythen from a minus character.
> Using a "string" such as X-AKAPPEND in the macro MAKE_CMD_ROW is a problem.
> astyle surounds the - with spaces.
> I propose to use a second macro MAKE_CMD_ROW2 providing the string as "X-AKAPPEND".
> It might be prettier to do this for all the commands.
> 
> The problem could also happens for the commands:
> X-AKLIST and X-AKLSUB
> 
> 
> Diffs
> -----
> 
>   server/tests/unittest/handlertest.cpp 6fc5d66 
> 
> Diff: https://git.reviewboard.kde.org/r/119576/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Guy Maurel
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/