Re: Cleanup before 2.3.0?

[Guenter Milde]

On 2017-07-15, Christian Ridderström wrote:
> On 7 July 2017 at 04:37, Scott Kostyshak  wrote:

>> > What do others think?

>> ^ If you get support from other LyX devs, and you are willing to take
>> care of everything, then I'm fine with it. My only other criterion is
>> that I don't want to personally spend any time on this. We have other
>> issues and debates going on where I want to spend my time. So the hard
>> part is convincing other LyX devs.

I support a cleanup before 2.3.0.

> I'm willing to take care of it and have been busy demonstrating it's safe
> to use clang-format. The "simple" task of verifying that a binary based on
> reformatted code is identical took much longer than expected and turned out
> to be non-trivial. E.g., building LyX twice in a row from the same source
> and from the same location didn't result the same binary on my mac. 

The idea of "reproducible builds" https://reproducible-builds.org/
may be of help here.
It has docs on "how to make your software build reproducibly…"
https://reproducible-builds.org/docs/.

Günter

Re: Cleanup before 2.3.0?

[Scott Kostyshak]

On Mon, Jul 17, 2017 at 12:10:31AM +0200, Christian Ridderström wrote:

> I just went through a large chunk of the minted postings and I still don't
> have a clear idea about my preference, and I'm therefore not sure what to
> write that'd contribute.
> 
> I'm generally inclined towards security and backwards compatibility.

Well, the problem is that it's not completely clear (at least to me)
which way forward is more secure. It's not that we're only talking about
trading off security versus a feature.

> my baseline is that I'd
> really hate it if I wasn't able to compile documents I wrote a long time
> ago.

+1

> I'd better stop writing now.

:)

Re: Cleanup before 2.3.0?

[Scott Kostyshak]

On Mon, Jul 17, 2017 at 12:48:34AM +0200, Enrico Forestieri wrote:

> ATM, in no way you can risk
> something if you decide to use minted. You would have to know what to
> change in the preferences for taking that risk. On the contrary, when
> using one of the above mentioned features, the risk is at the tip of
> a mouse click.

+1

Scott

Re: Cleanup before 2.3.0?

[Guillaume MM]

Le 15/07/2017 à 19:06, Jean-Marc Lasgouttes a écrit :

Le 15/07/2017 à 18:55, Christian Ridderström a écrit :
In my opinion, if we don't reach consensus easily on formatting 
issues, we should at least for now refrain from using a .clang-format.


This is probably what is going to take too much time...  I am not sure 
that we should have such a debate now.


Also regarding formatting, the available versions of clang-format 
might [*] Having done a few diffs when testing with clang-format I do 
find the use of TAB for indentation annoying. The reason is that 
sometimes the diff output in my terminal isn't aligned properly, i.e. 
what in reality is nicely aligned isn't aligned in my terminal. This 
was even after I configured Git to invoke 'less' with a tab size of 
width 4. So for this reason I'd actually prefer spaces to be used for 
indentation, but as tabs are what's currently used, that's a strong 
reason to stay with tabs.


The trick is to avoid using tabs for space alignment, like emacs' 
smart-tabs-mode does:

https://www.emacswiki.org/emacs/SmartTabs

I have to admit that I have not done it yes, and therefore I do that by 
hand currently.


Having taken the trouble of configuring this, I would recommend it. So I
am in favour of some automated code cleanup but only of the lightweight
kind. It should not touch indentation nor overlong lines which is
sometimes done by hand. There could be two levels of configuration: a
lightweight configuration that is applied to the code base and a more
complete one that people can use if they desire to. Does that make sense?

Guillaume

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 17 July 2017 at 00:48, Enrico Forestieri  wrote:

> Dear Christian,
>
> I see that the operated obfuscation of issues is working with you.
>

Dear Enrico,

Don't worry, all is not lost and any operation obfuscation has not yet
succeeded in invading Sweden. I still know that I don't know this topic
well enough to have a very useful opinion.

But perhaps better to continue discussing this in a more suitable thread?
Otherwise my plan for world domination by using clang-format to obfuscate
the entire LyX code base in one fell swoop will drown in discussions that
actually matter ;-)


> At the moment, there is no security problem with minted.
>
On the contrary
> there is a big question about security of features that were either
> present or recently introduced. Of course, it was the ability by which
> these questions were treated that lead you to think that the problem
> is minted, specifically, while it is not. ATM, in no way you can risk
> something if you decide to use minted.
>

It's good to know although I'm anyway not personally at risk, unfortunately
I'm currently not writing in LyX.

Cheers,
/Christian

Re: Cleanup before 2.3.0?

[Enrico Forestieri]

On Mon, Jul 17, 2017 at 12:10:31AM +0200, Christian Ridderström wrote:

> On 16 July 2017 at 22:45, Jean-Marc Lasgouttes  wrote:
> 
> > What I mean is that my absolute priority these days is to have 2.3.0 out.
> 
> 
> Fully understood.
> 
> 
> > The cleanups I proposed where chosen to have a minimal effect on release
> > date. Anything that requires too much thinking is a bit too much for me.
> > Currently minted and hyphen are blocking us, and we should work towards
> > solving this (even though these are touchy subjects).
> 
> 
> I just went through a large chunk of the minted postings and I still don't
> have a clear idea about my preference, and I'm therefore not sure what to
> write that'd contribute.
> 
> I'm generally inclined towards security and backwards compatibility.
> Perhaps it's because I experienced a directed attack at a previous
> workplace. Or perhaps it's an occupational hazard from previously working
> with satellite software as e.g. verification and validation manager. But
> even for that SW we took into account if there was a urgent and necessary
> need for e.g. intermediate release, assuming we had a realistic plan for
> fixing issues in a coming release.
> 
> For minted/hyphen, I'm e.g. not clear on the need for the features. The
> minted thing seems more optional, whereas the hyphen thing seems like a
> blocker. I haven't read the hyphen stuff yet, but my baseline is that I'd
> really hate it if I wasn't able to compile documents I wrote a long time
> ago.
> 
> Security scenarios/threat models for minted could be expanded upon. I mean
> that these days it's not just about me creating and editing my own
> document, instead authors collaborate and share the documents via e-mail
> and Dropbox etc. Theoretically some agency could intercept the document in
> transit and inject malicious code that they hope you'll execute on your
> computer.  Further, you might not be the direct target and instead its
> someone whose computer is on the same network as you. For instance, I've
> seen research papers from a swedish defence research institute written in
> LaTeX, or perhaps LyX, who knows. But if it was LyX then it could well be
> worth it for an adversary (Russia..) to compromise that author's computer.
> I'd better stop writing now.

Dear Christian,

I see that the operated obfuscation of issues is working with you.
At the moment, there is no security problem with minted. On the contrary
there is a big question about security of features that were either
present or recently introduced. Of course, it was the ability by which
these questions were treated that lead you to think that the problem
is minted, specifically, while it is not. ATM, in no way you can risk
something if you decide to use minted. You would have to know what to
change in the preferences for taking that risk. On the contrary, when
using one of the above mentioned features, the risk is at the tip of
a mouse click. So, please, stop with these FUD strategies (not directed
at you, of course) aimed at producing these results, namely mudding the
waters to confuse who don't have a clear view of the matter.

-- 
Enrico

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 16 July 2017 at 22:45, Jean-Marc Lasgouttes  wrote:

> What I mean is that my absolute priority these days is to have 2.3.0 out.


Fully understood.


> The cleanups I proposed where chosen to have a minimal effect on release
> date. Anything that requires too much thinking is a bit too much for me.
> Currently minted and hyphen are blocking us, and we should work towards
> solving this (even though these are touchy subjects).


I just went through a large chunk of the minted postings and I still don't
have a clear idea about my preference, and I'm therefore not sure what to
write that'd contribute.

I'm generally inclined towards security and backwards compatibility.
Perhaps it's because I experienced a directed attack at a previous
workplace. Or perhaps it's an occupational hazard from previously working
with satellite software as e.g. verification and validation manager. But
even for that SW we took into account if there was a urgent and necessary
need for e.g. intermediate release, assuming we had a realistic plan for
fixing issues in a coming release.

For minted/hyphen, I'm e.g. not clear on the need for the features. The
minted thing seems more optional, whereas the hyphen thing seems like a
blocker. I haven't read the hyphen stuff yet, but my baseline is that I'd
really hate it if I wasn't able to compile documents I wrote a long time
ago.

Security scenarios/threat models for minted could be expanded upon. I mean
that these days it's not just about me creating and editing my own
document, instead authors collaborate and share the documents via e-mail
and Dropbox etc. Theoretically some agency could intercept the document in
transit and inject malicious code that they hope you'll execute on your
computer.  Further, you might not be the direct target and instead its
someone whose computer is on the same network as you. For instance, I've
seen research papers from a swedish defence research institute written in
LaTeX, or perhaps LyX, who knows. But if it was LyX then it could well be
worth it for an adversary (Russia..) to compromise that author's computer.
I'd better stop writing now.

Cheers,
Christian

PS.
Scott, if you see this, let me take the opportunity to praise your work as
release manager -- you're doing an excellent job from what I've read!  It's
a shame you can't just setup a teleconference to discuss the issues.

Jean-Marc, in the best of all worlds there'd be a developer meeting planned
for the near future where these things could've been discussed face to
face.

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 16/07/2017 à 22:02, Christian Ridderström a écrit :

Indeed, my original message was about doing things that had a
moderate price tag attached to them. Imposing a style is IMO too
expensive for us at this point.


I'm definitely not looking to impose a new style and I'm spending time 
to get close to the "existing" formatting style.
I used quotations marks around "existing" as there's a bit of variation 
in what's currently used.


What I mean is that my absolute priority these days is to have 2.3.0 
out. The cleanups I proposed where chosen to have a minimal effect on 
release date. Anything that requires too much thinking is a bit too much 
for me. Currently minted and hyphen are blocking us, and we should work 
towards solving this (even though these are touchy subjects).


JMarc

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 16 July 2017 at 21:39, Jean-Marc Lasgouttes  wrote:

> Le 16/07/2017 à 21:34, Kornel Benko a écrit :
>
>> If not now, then probably never. There is no optimal start, except
>> at start of a project.
>>
>
> Not necessarily. We do not have much spare time to do it right, and this
> kind of thing needs to be correct on first run.
>

I'm not sure what you mean here.

Regarding formatting choices, I don't believe they have to be correct on
the first run. Or rather, I don't think they have to be "complete" on the
first run. You _do_ want to avoid large sets of changes that you later
revert. But I don't see it as a big drawback to e.g. initially use
clang-format without reformatting very long lines (we currently have >3500
lines that exceed 80 characters). At a later stage, we could then modify
the formatting configuration to get rid of long lines, or just do it
manually. Or leave them.


> We can discuss this in the 2.4 cycle, even if we decide to apply it only
> at the end of the cycle. Making sure that the style we choose can be
> obtained with everyone editor of choice is not a small affair.
>

Could you expand on the editor aspect?

I'm used to invoking clang-format from within Emacs on either the entire
buffer or on a specific region. Basically using clang-format has changed
how I code. These days while writing I've  stopped caring about whitespace
formatting. Instead I intermittently I do 'clang-format-buffer' to make it
pretty, e.g. before reviewing what I've done and definitely before commits.
I know people map some TAB-shortcut to invoke 'clang-format-region' easily.

I also know there's integration for clang-format for at least Emacs, Vim,
BBEdit, Visual Studio, XCode and Atom. Clang-format can of course also be
invoked from the command line. And it can be applied using e.g. 'git
clang-format' on only the parts that are being committed.

However, I'm _not_ expecting that most developers have to use clang-format.
Or even that they will...  If developers that regularly commit were to
start using it that'd be great.

I hope the info above helped.
/Christian

PS.
Note: I could create a CI job that warns if we start getting a lot of bad
formatting in commits. Or if you want to be tyrannical (not my
recommendation), you can have a pre-commit hook.




>
> JMarc
>
>

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 16 July 2017 at 21:15, Jean-Marc Lasgouttes  wrote:

> Le 16/07/2017 à 20:51, Scott Kostyshak a écrit :
>
>> "no debate" is good, but we want even more than that. We want a few more
>> "yes let's go for it!" before we impose a new style. I think we got
>> support from Kornel, but we would want more to go forward.  There are
>> other costs to changing many lines of the source code, such as "git
>> blame" not being as easy/helpful.
>>
>
> Indeed, my original message was about doing things that had a moderate
> price tag attached to them. Imposing a style is IMO too expensive for us at
> this point.


I'm definitely not looking to impose a new style and I'm spending time to
get close to the "existing" formatting style.
I used quotations marks around "existing" as there's a bit of variation in
what's currently used.

Here's my thinking/planning:

Once I'm close with a .clang-format file, I planned on assessing/presenting
how much impact applying clang-format would have on the code base, as well
as show what it'd look like. But also give some alternatives for
implementing it, e.g.:

- applying to all files at once

- for now only apply to source that haven't been modified in a long time

- for now only apply to source where there's only minor/trivial formatting
change
  (this is conceptually similar to Richard's removal of trailing whitespace)

- only add the .clang-format, letting developers use it as they please when
preparing patches

- suggest use of clang-format as part of a pre-commit hook, either on an
entire source file or only on the changed parts.
  (there's a cute command: git clang-format)

I need to think/read a little more on these alternatives, and see how
they'd interact with the code before I could recommend one.

Speaking of reading/thinking, Scott's right about that there can be a cost
associated:
- the time I'm putting in, which is quite a few hours actually but I'm
enjoying it and learning things
- effect on e.g. "git blame"
- risk of introducing errors (I think I've got that covered though)
- cost for developers with pending work in their own branches
- cost of issues related to differences between versions of clang-format
  (I'm currently looking at this, by comparing the result of using ver. 3.8
vs. 3.9 vs 4.0 vs. 5.0)
- ?

/Christian

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 16/07/2017 à 21:34, Kornel Benko a écrit :

If not now, then probably never. There is no optimal start, except
at start of a project.


Not necessarily. We do not have much spare time to do it right, and this 
kind of thing needs to be correct on first run.


We can discuss this in the 2.4 cycle, even if we decide to apply it only 
at the end of the cycle. Making sure that the style we choose can be 
obtained with everyone editor of choice is not a small affair.


JMarc

Re: Cleanup before 2.3.0?

[Kornel Benko]

Am Sonntag, 16. Juli 2017 um 21:15:42, schrieb Jean-Marc Lasgouttes 

> Le 16/07/2017 à 20:51, Scott Kostyshak a écrit :
> > "no debate" is good, but we want even more than that. We want a few more
> > "yes let's go for it!" before we impose a new style. I think we got
> > support from Kornel, but we would want more to go forward.  There are
> > other costs to changing many lines of the source code, such as "git
> > blame" not being as easy/helpful.
> 
> Indeed, my original message was about doing things that had a moderate 
> price tag attached to them. Imposing a style is IMO too expensive for us 
> at this point.
> 
> JMarc

If not now, then probably never. There is no optimal start, except
at start of a project.

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 16/07/2017 à 20:51, Scott Kostyshak a écrit :

"no debate" is good, but we want even more than that. We want a few more
"yes let's go for it!" before we impose a new style. I think we got
support from Kornel, but we would want more to go forward.  There are
other costs to changing many lines of the source code, such as "git
blame" not being as easy/helpful.


Indeed, my original message was about doing things that had a moderate 
price tag attached to them. Imposing a style is IMO too expensive for us 
at this point.


JMarc

Re: Cleanup before 2.3.0?

[Scott Kostyshak]

On Sat, Jul 15, 2017 at 07:53:29PM +0200, Jean-Marc Lasgouttes wrote:
> Le 15/07/2017 à 19:28, Christian Ridderström a écrit :
> > I hope we can at least see if a debate is necessary, we might get lucky.
> > Perhaps the current source code is mainly consistent, in which case we
> > could align to that format. I can at least compile a list of topics.
> 
> Sure. What I do not want to see is, in very nested loops, function calls
> split to a ridiculous number of line to avoid the 72 (80?) characters limit.

"no debate" is good, but we want even more than that. We want a few more
"yes let's go for it!" before we impose a new style. I think we got
support from Kornel, but we would want more to go forward.  There are
other costs to changing many lines of the source code, such as "git
blame" not being as easy/helpful.

Scott


signature.asc
Description: PGP signature

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 15/07/2017 à 19:28, Christian Ridderström a écrit :
I hope we can at least see if a debate is necessary, we might get 
lucky.  Perhaps the current source code is mainly consistent, in which 
case we could align to that format. I can at least compile a list of topics.


Sure. What I do not want to see is, in very nested loops, function calls 
split to a ridiculous number of line to avoid the 72 (80?) characters 
limit.



Btw, what (if any) version of clang-format do you have on your system?
/Christian


On Ubuntu 17.04: 4.0
On Ubuntu 16.04: 3.8

We should see what features are really required for what we want to do.

Clang format uses TAB for block indentation and spaces for the rest, 
i.e. Emacs smart-tabs, through the setting


 UseTabStyle: ForIndentation


Good. I like this one.

JMarc

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 15 July 2017 at 19:06, Jean-Marc Lasgouttes  wrote:

> Le 15/07/2017 à 18:55, Christian Ridderström a écrit :
>
>> In my opinion, if we don't reach consensus easily on formatting issues,
>> we should at least for now refrain from using a .clang-format.
>>
>
> This is probably what is going to take too much time...  I am not sure
> that we should have such a debate now.
>

I hope we can at least see if a debate is necessary, we might get lucky.
Perhaps the current source code is mainly consistent, in which case we
could align to that format. I can at least compile a list of topics.

Btw, what (if any) version of clang-format do you have on your system?
/Christian

PS.

>
> The trick is to avoid using tabs for space alignment, like emacs'
> smart-tabs-mode does:
> https://www.emacswiki.org/emacs/SmartTabs
>
> I have to admit that I have not done it yes, and therefore I do that by
> hand currently.


Clang format uses TAB for block indentation and spaces for the rest, i.e.
Emacs smart-tabs, through the setting

UseTabStyle: ForIndentation

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 15/07/2017 à 18:55, Christian Ridderström a écrit :
In my opinion, if we don't reach consensus easily on formatting issues, 
we should at least for now refrain from using a .clang-format.


This is probably what is going to take too much time...  I am not sure 
that we should have such a debate now.


Also regarding formatting, the available versions of clang-format might 
[*] Having done a few diffs when testing with clang-format I do find the 
use of TAB for indentation annoying. The reason is that sometimes the 
diff output in my terminal isn't aligned properly, i.e. what in reality 
is nicely aligned isn't aligned in my terminal. This was even after I 
configured Git to invoke 'less' with a tab size of width 4. So for this 
reason I'd actually prefer spaces to be used for indentation, but as 
tabs are what's currently used, that's a strong reason to stay with tabs.


The trick is to avoid using tabs for space alignment, like emacs' 
smart-tabs-mode does:

https://www.emacswiki.org/emacs/SmartTabs

I have to admit that I have not done it yes, and therefore I do that by 
hand currently.


JMarc

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 7 July 2017 at 04:37, Scott Kostyshak  wrote:

> > What do others think?
>
> ^ If you get support from other LyX devs, and you are willing to take
> care of everything, then I'm find with it. My only other criterion is
> that I don't want to personally spend any time on this. We have other
> issues and debates going on where I want to spend my time. So the hard
> part is convincing other LyX devs.
>

Hi Scott,

I'm willing to take care of it and have been busy demonstrating it's safe
to use clang-format. The "simple" task of verifying that a binary based on
reformatted code is identical took much longer than expected and turned out
to be non-trivial. E.g., building LyX twice in a row from the same source
and from the same location didn't result the same binary on my mac.  So I
had to come up with an alternative approach based on doing pair-wise
comparisons of the disassembled code of object files.

The principle is that if two sets of source code results in object files
with the same assembler code, then the two sets are equivalent code wise. I
think it's a pretty solid approach. I'm going to post the details
separately to let developers assess if they think the approach sufficiently
well demonstrate that it's safe to use clang-format.

I used the approach only on macOS and in essence I produced two sets of
source code files and then built them separately. Then for each pair of
object files I did the following:
  - Strip symbols (strip -u -r) from the two object files
  - Disassemble (objdump -d) the two object files into assembly code
  - Compare the assembly code of the two object files

It was educational and I had to do some workarounds, but my conclusion is
that it's sufficiently safe to apply clang-format, with the configuration I
created.
If deemed necessary I could repeat the process on Linux, but probably not
on Windows.

Regarding code formatting, I think a style used consistently and
_automatically_ over the source code is much more important than a
particular style choice [*].

In 2007 Bo Peng added a uncrustify.cfg which _perhaps_ represents the
canonical "LyX coding style", but who knows. I will look at that for
guidance, and also at the existing code. However, sometimes I
might/should/ought to ask for consensus regarding a choice between
formatting styles. Especially as developers can have surprisingly strong
opinions here in my experience. So there's a risk of time consuming
discussions... but it's up to you to stay away ;-)

In my opinion, if we don't reach consensus easily on formatting issues, we
should at least for now refrain from using a .clang-format.

Also regarding formatting, the available versions of clang-format might
result in formatting restrictions [**].

With consensus on the format, and thus a different configuration file for
clang-format, I would repeat the check that globally applying clang-format
does not change the code. One reason for this is that I did notice that
changing the order of include files did result in slightly different
results (the difference might have been benign of course).

Best regards,
/Christian

[*] Having done a few diffs when testing with clang-format I do find the
use of TAB for indentation annoying. The reason is that sometimes the diff
output in my terminal isn't aligned properly, i.e. what in reality is
nicely aligned isn't aligned in my terminal. This was even after I
configured Git to invoke 'less' with a tab size of width 4. So for this
reason I'd actually prefer spaces to be used for indentation, but as tabs
are what's currently used, that's a strong reason to stay with tabs.

[**] I'll ask developers what version of clang-format they have available,
or could easily install. Even though later versions of clang-format have
some neat features, it's bad for us if the project's .clang-format cannot
be used by all developers, or if it doesn't produce the same result. This
matters for the developers what would actually be using clang-format.

On my mac, 'brew install clang-format' gave me ver. 4.0.0, but doing  'brew
upgrade clang-format' gave me version 5.0.0 which is the latest version.

Re: Cleanup before 2.3.0?

[Kornel Benko]

Am Freitag, 7. Juli 2017 um 00:03:50, schrieb Christian Ridderström 

> 
> PS.
> As I see it, after we're using clang-format, a next step could be to use
> 'clang-tidy' to do some static analysis.
> clang-format-custom

I have version clang-format-3.9. Using this version, there are many differences 
to
our .cpp files.

The most annoying are created by
AlignAfterOpenBracket: Align,
I propose here
AlignAfterOpenBracket: DontAlign

Example from VCBackend.cpp:64
Orig:
frontend::Alert::error(_("Revision control error."),
bformat(_("Some problem occurred while running the 
command:\n"
  "'%1$s'."),
from_utf8(cmd)));

Align:
frontend::Alert::error(_("Revision control error."),
   bformat(_("Some 
problem occurred while running the command:\n"

 "'%1$s'."),
   
from_utf8(cmd)));
DontAlign:

frontend::Alert::error(_("Revision control error."),
bformat(_("Some problem occurred while running the 
command:\n"
  "'%1$s'."),
from_utf8(cmd)));


Kornel



signature.asc
Description: This is a digitally signed message part.

Re: Cleanup before 2.3.0?

[Kornel Benko]

Am Donnerstag, 6. Juli 2017 um 22:37:59, schrieb Scott Kostyshak 

> On Fri, Jul 07, 2017 at 12:03:50AM +0200, Christian Ridderström wrote:
> 
> > - We can compare the built executable before and after running clang-format,
> >   the executables should be identical.
> 
> I don't know why I had never considered that approach before (although
> it is obvious now), and I like this idea.
> 
> > > Even if no functionality is changed in
> > > theory, we could somehow expose a compiler bug (this is true even if
> > > just changing whitespace).
> > >
> > 
> > I'd expect this to in practice be covered by binary comparison of the
> > executable.
> 
> +1
> 
> > In theory we could of course still be hit by a compiler bug on a platform
> > where we don't compare the executable.
> > 
> > Also, anyone doing the same whitespace change manually would trigger such a
> > bug...  in theory the removal of trailing whitespace from lines could
> > trigger it.
> 
> +1
> 
> > What do others think?
> 
> ^ If you get support from other LyX devs, and you are willing to take
> care of everything, then I'm find with it. My only other criterion is
> that I don't want to personally spend any time on this. We have other
> issues and debates going on where I want to spend my time. So the hard
> part is convincing other LyX devs.
> 
> Scott

I support this approach.

Kornel

signature.asc
Description: This is a digitally signed message part.

Re: Cleanup before 2.3.0?

[Scott Kostyshak]

On Fri, Jul 07, 2017 at 12:03:50AM +0200, Christian Ridderström wrote:

> - We can compare the built executable before and after running clang-format,
>   the executables should be identical.

I don't know why I had never considered that approach before (although
it is obvious now), and I like this idea.

> > Even if no functionality is changed in
> > theory, we could somehow expose a compiler bug (this is true even if
> > just changing whitespace).
> >
> 
> I'd expect this to in practice be covered by binary comparison of the
> executable.

+1

> In theory we could of course still be hit by a compiler bug on a platform
> where we don't compare the executable.
> 
> Also, anyone doing the same whitespace change manually would trigger such a
> bug...  in theory the removal of trailing whitespace from lines could
> trigger it.

+1

> What do others think?

^ If you get support from other LyX devs, and you are willing to take
care of everything, then I'm find with it. My only other criterion is
that I don't want to personally spend any time on this. We have other
issues and debates going on where I want to spend my time. So the hard
part is convincing other LyX devs.

Scott


signature.asc
Description: PGP signature

Re: Cleanup before 2.3.0?

[Christian Ridderström]

Hi Scott,

On 6 July 2017 at 22:20, Scott Kostyshak  wrote:

> > - Now (before release) would be a good time to start using clang-format
>
> Why? One reason is that it might make it easier to backport fixes from
> master to 2.3.x.


This was my reason, as comparison of code becomes easier through 2.3.x.
Other than that we can certainly do it at a later stage.


> A reason against such a change is that the benefit is
> more long-run than short-run, as there is always the risk that unwanted
> functionality could change.


We should certainly consider the risk of changed functionality, but we are
actually quite safe:

- With one exception (see below) clang-format is designed to only change
   "whitespace" of the source code, i.e. adding/removing whitespace.

- We can compare the built executable before and after running clang-format,
  the executables should be identical.

- The one exception is if clang-format is allowed to sort the order of
"includes".
  But we disallow this (SortIncludes: false), so it won't happen.
  Note: Good source code should not depend on the order of the includes,
but ...

- Strictly speaking the behaviour of the program can change through a macro
  like __LINE__, if we have some crazy code that depends on a piece of
  code being located at some specific line number.  Otherwise we're talking
about
  that line numbers in a possible log output might be different. Just as if
someone
  had manually added or removed a few source code lines.

- If other programs read the LyX source code and depend on certain
  things being located on certain lines, or at certain indentation levels,
this
  could affect the overall functionality.
  So Doxygen would need to be checked that it still works - if we actually
use it for anything?


> Even if no functionality is changed in
> theory, we could somehow expose a compiler bug (this is true even if
> just changing whitespace).
>

I'd expect this to in practice be covered by binary comparison of the
executable.
In theory we could of course still be hit by a compiler bug on a platform
where we don't compare the executable.

Also, anyone doing the same whitespace change manually would trigger such a
bug...  in theory the removal of trailing whitespace from lines could
trigger it.

What do others think?


I'm attaching a file that should be saved as ".clang_format" in e.g. src/
or a in a higher directory.
This will then automatically be used by clang-format. To apply clang-format
to all files in src/, run:
  cd src
  clang-format -i *.cpp *.h

Note: The same would have to be done for source code in the subfolders as
well.

Note: From within Emacs you can do 'M-x clang-format-buffer'.

Note: I've tested with clang-format version 4.0. I don't know what the
minimum required version number is for the configuration settings I'm using.

Note: I created the attached .clang-format to try and minimise the number
of changed source code lines.
This configuration should be seen as a first step, to let people see what
we're talking about.
In particular, it's currently typically not fixing lines that are to long.

/Christian

PS.
As I see it, after we're using clang-format, a next step could be to use
'clang-tidy' to do some static analysis.


clang-format-custom
Description: Binary data

Re: Cleanup before 2.3.0?

[Scott Kostyshak]

On Thu, Jul 06, 2017 at 08:23:24PM +0200, Christian Ridderström wrote:

> - Now (before release) would be a good time to start using clang-format

Why? One reason is that it might make it easier to backport fixes from
master to 2.3.x. A reason against such a change is that the benefit is
more long-run than short-run, as there is always the risk that unwanted
functionality could change. Even if no functionality is changed in
theory, we could somehow expose a compiler bug (this is true even if
just changing whitespace).

What do others think?

Scott

Re: Cleanup before 2.3.0?

[Christian Ridderström]

On 3 July 2017 at 11:26, Jean-Marc Lasgouttes  wrote:

> Since we are approaching major release, I think it is a good time to do
> some mechanical clean-ups. The idea is that it is better to do it now
> instead of at the beginning of a cycle in order to ease backporting of
> patches to stable.
>

I posted what's below in a different thread, but it's better of here:

-
If we're open to doing to some cleanup of the code I strongly suggest we
use a tool called 'clang-format' to automatically take care of code
formatting. I've introduced this tool at my work and it's _really_ nice to
not have to care about the formatting in most cases.  Tonight I'll create a
"format" file, ".clang-format", that attempts to replicate the formatting
style of the typical LyX code. If that goes well we can then discuss if it
makes sense to use/apply clang-format to the LyX source code. It might well
be that it results in too many changes for us to want to apply them.

Note: I have seen there's a config file for uncrustify in the repository,
but I got some error messages when trying to run uncrustify on the LyX
code.  Running clang-format on the LyX code works fine though. The
difficult part is tailoring the formatting rules to the current formatting
style to minimise the number of changes, and to avoid tedious discussions
about how the code ought to be formatted.

In case you're interested in clang-format, there's plenty of information
online. Chandler Carruth who leads the C++, Clang, and LLVM teams at
Google has a nice talk that includes a live demo of using clang-format in
this video: https://www.youtube.com/watch?v=cX_GhJ6BuWI. The demo starts as
about 29 minutes, but I'd suggest looking from 23 minutes. Fun detail:
Chandler references TeX and the issue of where to break lines, clang-format
needs to do something similar to the source code.
-

Expanding on the above.
- IMHO it's really nice to use clang-format
- Now (before release) would be a good time to start using clang-format
- It has integrations for various IDEs and editors, but can also be run
from the command line.
- It's some work to start using it
  - Trying to replicate the "nominal" formatting style that the old code
uses.
 I will make an attempt here tonight.
  - Realising that a lot of the old code didn't actually use your nominal
formatting style.

And something really important.  I think Scott, as release manager, has
absolute power to veto the use of clang-format before a release. Or to
limit the extent to which it is used. Especially if a never-ending
discussion on code formatting starts...

Cheers,
Christian

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 03/07/2017 à 11:26, Jean-Marc Lasgouttes a écrit :
* renaming MathMacro (resp. MathMacroTemplate) to InsetMathMacro (resp. 
InsetMathMacroTemplate). These are insets, and therefore they should be 
named as insets. It helps understanding mathed sources IMO (I have been 
looking a lot at these files recently).


I did that, plus a few other classes. Now all children of Inset have a 
name that starts with "Inset".


JMarc

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 03/07/17 à 20:48, Richard Heck a écrit :

I left the *.ui files, since these are (typically) auto-generated, and
Qt Designer will do as it pleases.


Good point :)

JMarc

Re: Cleanup before 2.3.0?

[Richard Heck]

On 07/03/2017 02:09 PM, Jean-Marc Lasgouttes wrote:
> Le 03/07/17 à 19:58, Richard Heck a écrit :
>> These are the files that still have trailing whitespace:
>
> Makefile.am, *.m files and tex2lyx/tex2lyx.1in can be trimmed. *ui
> file probably too.

I left the *.ui files, since these are (typically) auto-generated, and
Qt Designer will do as it pleases.

>>
>> PS From src/:
>> perl -pe 's/\s+\n$/\n/' $(find ./ -type f -name '*.cpp')
>> and similarly for other file types.
>
> What is the \n$ good for?

I guess I could have left that out. It's the \n in the replacement that
matters. You can imagine the chaos of my first attempt. I always forget
that perl treats \n as whitespace at the end of the line

Richard

Re: Cleanup before 2.3.0?

[Jean-Marc Lasgouttes]

Le 03/07/17 à 19:58, Richard Heck a écrit :

These are the files that still have trailing whitespace:


Thanks for doiing it, Richard.

Makefile.am, *.m files and tex2lyx/tex2lyx.1in can be trimmed. *ui file 
probably too.


I think that *.lyx files should not be touched, since spaces can be at 
the end of lines (this is a bad file format).


The test files can probably not be changed, but we can have a look at 
the reason for thetrailing spaces.




PS From src/:
perl -pe 's/\s+\n$/\n/' $(find ./ -type f -name '*.cpp')
and similarly for other file types.


What is the \n$ good for?

JMarc

Re: Cleanup before 2.3.0?

[Richard Heck]

On 07/03/2017 05:26 AM, Jean-Marc Lasgouttes wrote:
>
> Hi there,
>
> Since we are approaching major release, I think it is a good time to
> do some mechanical clean-ups. The idea is that it is better to do it
> now instead of at the beginning of a cycle in order to ease
> backporting of patches to stable.
>
> I have two things in mind:
>
> * renaming MathMacro (resp. MathMacroTemplate) to InsetMathMacro
> (resp. InsetMathMacroTemplate). These are insets, and therefore they
> should be named as insets. It helps understanding mathed sources IMO
> (I have been looking a lot at these files recently).
>
> * remove all trailing spaces in source code. This is something we
> should not have to do during normal code work. I am not sure though
> what a good tool would be to do that (although I am sure that emacs
> will not fail me :).
>
> As you can see, none of this is a "must do", but if you come up with
> other changes, we could discuss them too.

Yes, I think these are wise things to do.

Regarding the trailing spaces, I have just done it. Surely that is safe
enough. I've verified with 'git diff --ignore-space-at-eol' that only
such whitespace was changed.

These are the files that still have trailing whitespace:

frontends/qt4/Makefile.am
frontends/qt4/ui/CharacterUi.ui
frontends/qt4/ui/MarginsUi.ui
frontends/tests/test_biblio.cmake
support/AppleScript.m
support/AppleSpeller.m
support/linkback/LinkBack.m
support/linkback/LinkBackProxy.m
support/linkback/LinkBackServer.m
support/tests/supporttest.cmake
tests/regfiles/ExternalTransforms
tests/regfiles/Length
tex2lyx/test/CJK.lyx.lyx
tex2lyx/test/CJK.tex
tex2lyx/test/CJKutf8.lyx.lyx
tex2lyx/test/CJKutf8.tex
tex2lyx/test/DummyDocument.lyx.lyx
tex2lyx/test/Dummy~Document.lyx.lyx
tex2lyx/test/XeTeX-polyglossia.lyx.lyx
tex2lyx/test/algo2e.lyx.lyx
tex2lyx/test/algo2e.tex
tex2lyx/test/box-color-size-space-align.lyx.lyx
tex2lyx/test/box-color-size-space-align.tex
tex2lyx/test/foo.png
tex2lyx/test/test-insets-basic.lyx.lyx
tex2lyx/test/test-insets-basic.tex
tex2lyx/test/test-insets.lyx.lyx
tex2lyx/test/test-insets.tex
tex2lyx/test/test-minted.lyx.lyx
tex2lyx/test/test-minted.tex
tex2lyx/test/test-modules.lyx.lyx
tex2lyx/test/test-refstyle-theorems.lyx.lyx
tex2lyx/test/test-structure.lyx.lyx
tex2lyx/test/test-structure.tex
tex2lyx/test/test.lyx.lyx
tex2lyx/test/verbatim.lyx.lyx
tex2lyx/test/xfigtest.fig
tex2lyx/test/xfigtest.pstex
tex2lyx/tex2lyx.1in

I'm not sure which if any of these can or should be changed. Probably
not the files in tex2lyx/test/.

Richard

PS From src/:
perl -pe 's/\s+\n$/\n/' $(find ./ -type f -name '*.cpp')
and similarly for other file types.