Re: [lmi] PATCH: C++20 and clang

[Greg Chicares]

On 4/20/21 12:56 AM, Vadim Zeitlin wrote:
> On Tue, 20 Apr 2021 00:38:45 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> Indeed I do have 'origin' configured to point to savannah.
> GC> But I hadn't pulled from savannah since yesterday, so I
> GC> was probably comparing your branch to local master.
> 
>  Yes, but I still don't understand why there was no mention of
> origin/master update in "git fetch --all" output.

Probably I copied only the part that seemed relevant to me at
the time. Scrolling back, I see:

/opt/lmi/src/lmi[0]$git fetch --all         
Fetching origin
remote: Counting objects: 18, done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 18 (delta 8), reused 0 (delta 0)
Unpacking objects: 100% (18/18), done.
>From git://git.savannah.nongnu.org/lmi
   8be683cc1..9414edf44  master     -> origin/master
Fetching xanadu
remote: Enumerating objects: 26, done.
remote: Counting objects: 100% (26/26), done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 19 (delta 13), reused 18 (delta 12), pack-reused 0
Unpacking objects: 100% (19/19), done.
>From https://github.com/vadz/lmi
 * [new branch]          clang-cxx20 -> xanadu/clang-cxx20
   40c119dea..9414edf44  master      -> xanadu/master
Fetching shangri-la
/opt/lmi/src/lmi[0]$git log -6 xanadu/clang-cxx20 

so I think this is what you expected to see:
   8be683cc1..9414edf44  master     -> origin/master
and it was indeed on my screen, but it wasn't what I was
looking for, so I ignored it.

> GC> I believe I've merged everything now.
> 
>  Thanks a lot! Unfortunately the first commit of this series, i.e.
> 
> https://github.com/let-me-illustrate/lmi/pull/179/commits/9a4888100bbf4029c72fc9c598122079674dcb9f
> 
> seems to be missing from master. I don't think it was intentional and while
> it doesn't break the CI build any longer (because later commits disabled
> the warning this code generated with clang in C++20 mode), I believe it
> would still be nice to apply it. Could you please do it?

Yes, it's a nice change--clang's complaint here is exactly the
sort of complaint I want a compiler to make--and I'll apply it.

But first I want to understand how I missed it. Could it have
been applied, but then combined with another commit in one of
several git-rebase-interactive cycles? No, it's not present at all.

I did this, after applying all but one of the patches:

  $git cherry-pick xanadu/clang-cxx20
  On branch master
  Your branch is up to date with 'origin/master'.

  You are currently cherry-picking commit e1ffd97fe.

but that's the "volatile" patch, so I did

  git cherry-pick --skip

and it seemed to succeed, leading me to think I had gotten them all.
But the command was wrong--I forgot the two dots:

-  $git cherry-pick xanadu/clang-cxx20
+  $git cherry-pick ..xanadu/clang-cxx20

so now I go back and run the correct command:

  $git cherry-pick ..xanadu/clang-cxx20
  [master eb5f1ef56] Use constexpr instead of enum for defining constants
   Author: Vadim Zeitlin <vadim@tt-solutions.com>
   Date: Mon Apr 19 13:34:00 2021 +0200
   1 file changed, 2 insertions(+), 2 deletions(-)
  On branch master
  Your branch is ahead of 'origin/master' by 1 commit.
    (use "git push" to publish your local commits)

  Cherry-pick currently in progress.

  Untracked files: [...edited out...]

  nothing added to commit but untracked files present
  The previous cherry-pick is now empty, possibly due to conflict resolution.
  If you wish to commit it anyway, use:

      git commit --allow-empty

  and then use:

      git cherry-pick --continue

so I tried that, but soon this happened:

  $git cherry-pick --skip
  Auto-merging configure.ac
  CONFLICT (content): Merge conflict in configure.ac
  error: could not apply e4cb44b01... Disable clang enum-related warnings in 
C++20 mode
  hint: after resolving the conflicts, mark the corrected paths
  hint: with 'git add <paths>' or 'git rm <paths>'
  hint: and commit the result with 'git commit'

and no, I'm not going through all of that. Ideally I would have
written the two dots very early in the process, but it's just too
laborious to repeat the whole thing now, with all the insertions
and fixups.

I try to avoid the labor with

  $git cherry-pick --skip    
  $git cherry-pick --continue
  [repeat]

but that just leads me to:

  error: no cherry-pick or revert in progress
  fatal: cherry-pick failed
  [return code 128]

which looks hopeless, but when I look where I've ended up...

  $git status
  On branch master
  Your branch is ahead of 'origin/master' by 1 commit.
    (use "git push" to publish your local commits)

...it looks like exactly where I want to be.

There's nothing ugly in git-status, git-log shows only what
should be there, and all the tests pass...so now I push.

> GC> I did some fixups to make 'make $coefficiency check_concinnity'
> GC> succeed.
> 
>  Oops, really sorry about this, I should have rerun it. But I don't
> understand how did the CI builds pass, they're supposed to check for
> this... oh, now I see: the problem was indeed detected, see
> 
> https://github.com/let-me-illustrate/lmi/runs/2382487360?check_suite_focus=true#step:22:25
> 
> but apparently "make check_concinnity" doesn't exit with error even when it
> does detect problems. This is really unexpected and counter-intuitive.
> Would you consider accepting patches changing this?

I'm loth to touch it, and AFAICT there's no need to touch it
because ./hooks/pre-commit already does what you want.

The hook does have one problem, which I haven't addressed yet
because, while I'm somewhat less loth to touch that one, it
doesn't seem trivial or fun. The problem is that if the
'test_coding_rules' binary needs to be rebuilt, the hook
reports failure even when it will succeed once rebuilt,
because rebuilding it spews out rebuilding commands that the
hook does not expect. I'd be glad if you want to patch that.

> GC> >  In any case, the commits that are part of this PR are the first 6 
> commits
> GC> > in the list above (they're also those that appear on the web page if you
> GC> > open the URL above in the browser).
> GC> From your previous messages, it seems that you took extra
> GC> pains to present this patchset in various ways to help me.
> GC> That's appreciated, but it's not really necessary as I
> GC> don't take advantage of all the diversity. Let me explain
> GC> what I did here, because it may save you time in future.
> 
>  My explanations are just meant to be complimentary to the commit messages
> and also mention things that I didn't do even thought they might be worth
> doing. I.e. I do understand that you apply the commits locally and not
> using web UI, the links to the commits on GitHub are just for reference. I
> thought that having the extra information in the post could be useful,
> compared to just having the commit messages themselves, but if you think
> this is too much and that commit messages are enough, I could avoid writing
> these additional explanations, of course.
> 
>  Please let me know what would you prefer in the future,

Well...I deliberately try not to read those explanations, because
skimming them sometimes makes me fear that a ponderous change
is being proposed, when applying the changes shows it to be small.
For example, I really don't want to step back and think about
replacing C++98 enums with constexpr globally, because that sounds
like a vast and deep undertaking for little gain, which I ought to
avoid even thinking about; but I'm really glad to change a couple
of lines in a way that's an obvious improvement.