[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-pim
Subject: Re: [Kde-pim] Review Request 119576: how to protect a command from a change by using astyle
From: "David Jarvie" <djarvie () kde ! org>
Date: 2014-08-04 16:33:56
Message-ID: 20140804163356.20973.4659 () probe ! kde ! org
[Download RAW message or body]
> On Aug. 4, 2014, 1: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 ''.
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.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119576/#review63774
-----------------------------------------------------------
On Aug. 2, 2014, 5: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, 5: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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic