[Libreoffice-commits] core.git: 7 commits - chart2/source extensions/source hwpfilter/source l10ntools/source sal/osl unoidl/source

[Norbert Thiebaud]

On Mon, Aug 4, 2014 at 2:51 PM, Stephan Bergmann <sbergman at redhat.com> wrote:
>
> I don't understand your problem.  Do you mean, you're unable to read code
> formatted as
>
>> bool operator ==(ConstantValue const & lhs, ConstantValue const & rhs) {
>>     if (lhs.type == rhs.type) {
>>         switch (lhs.type) {
>
>
> rather than as
>
>> bool operator ==(ConstantValue const & lhs, ConstantValue const & rhs)
>> {
>>     if (lhs.type == rhs.type)
>>     {
>>         switch (lhs.type)
>>         {
>

yes I find it very very hard to follow... especially matching the closing }
to me if feel as awkward to read as a right aligned text with a
hanging indent on the right.

for a very small block I can managed, but with enough lines between
the opening and the ending of a block, especially with
mutiple level of nesting in between.. it become quite hard to 'see'
the flow, and I end-up re-formatting just to see the code structure.
Now add to that the sometime extreme verbosity of c++ that turn a
simple for(;;) into a 4 or 5 lines animal, and it is clear why having
a { on a single line is much much more clearer.
Point in case is that you felt apparently compeleed to switch to that
mode when faced with such for() or if() see example below
cut-and-pasted from the 'original' source in question

>  And
> especially so if a single file out of a coherent set of consistently
> formatted files is reformatted differently.
>

well it was not as illustrated by the snipset below:

        case unoidl::Entity::SORT_CONSTANT_GROUP:
            {
                rtl::Reference<unoidl::ConstantGroupEntity> ent2B(
                    static_cast<unoidl::ConstantGroupEntity *>(entB.get()));
                for
(std::vector<unoidl::ConstantGroupEntity::Member>::const_iterator
                             i(ent2B->getMembers().begin());
                     i != ent2B->getMembers().end(); ++i)
                {
                    bool found = false;
                    if (entA.is()) {
                        rtl::Reference<unoidl::ConstantGroupEntity> ent2A(
                            static_cast<unoidl::ConstantGroupEntity *>(
                                entA.get()));
                        for
(std::vector<unoidl::ConstantGroupEntity::Member>::const_iterator
                                 j(ent2A->getMembers().begin());
                             j != ent2A->getMembers().end(); ++j)
                        {
                            if (i->name == j->name) {
                                found = true;
                                break;
                            }
                        }
                    }
                    if (!(found || valid(i->name))) {
                        std::cerr
                            << "Constant group " << name << " member "
                            << i->name << " uses an invalid identifier"
                            << std::endl;
                        std::exit(EXIT_FAILURE);
                    }
                }
                break;
            }

note the for() and case syntax use { below on a new line.. the if use
{ at the end of the line

or
            case unoidl::Entity::SORT_SINGLE_INTERFACE_BASED_SERVICE:
                {
                    rtl::Reference<unoidl::SingleInterfaceBasedServiceEntity>
                        ent2A(

static_cast<unoidl::SingleInterfaceBasedServiceEntity *>(
                                entA.get()));
                    rtl::Reference<unoidl::SingleInterfaceBasedServiceEntity>
                        ent2B(

static_cast<unoidl::SingleInterfaceBasedServiceEntity *>(
                                entB.get()));
                    if (ent2A->getBase() != ent2B->getBase()) {
                        std::cerr
                            << "single-interface--based servcie " << name
                            << " base changed from " << ent2A->getBase()
                            << " to " << ent2B->getBase()
                            << std::endl;
                        std::exit(EXIT_FAILURE);
                    }
                    if (ent2A->getConstructors().size()
                        != ent2B->getConstructors().size())
                    {
                        std::cerr
                            << "single-interface--based service " << name
                            << " number of constructors changed from "
                            << ent2A->getConstructors().size() << " to "
                            << ent2B->getConstructors().size() << std::endl;
                        std::exit(EXIT_FAILURE);
                    }
                    for
(std::vector<unoidl::SingleInterfaceBasedServiceEntity::Constructor>::const_iterator
                             i(ent2A->getConstructors().begin()),
                             j(ent2B->getConstructors().begin());
                         i != ent2A->getConstructors().end(); ++i, ++j)
                    {
                        if (i->name != j->name || i->parameters != j->parameters
                            || i->exceptions != j->exceptions
                            || i->defaultConstructor != j->defaultConstructor)
                        {
                            std::cerr
                                << "single-interface--based service " << name
                                << " constructor #"
                                << i - ent2A->getConstructors().begin() + 1
                                << " changed from "
                                << (i->defaultConstructor
                                    ? OUString("default ") : i->name)
//TODO: parameters, exceptions
                                << " to "
                                << (j->defaultConstructor
                                    ? OUString("default ") : j->name)
//TODO: parameters, exceptions
                                << std::endl;
                            std::exit(EXIT_FAILURE);
                        }
                    }
                    break;
                }

not the mix of if() format.

So, no, the source was not "consistently formatted"

> was not meant to say "and now I'm angry" but rather "and that's why I noted this change in the first place."

While we are on clarification :-) I did not change it to annoy you,
but because I needed to read the context of the entire function and
even more to try to figure out what the intent was wrt to what
coverity was complaining about.... and since it was a mix bag of style
to start with...

Norbert