This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove "// -*- C++ -*-" comments from all .cpp files. NFCI.
ClosedPublic

Authored by Quuxplusone on Sep 30 2021, 12:46 PM.

Details

Reviewers
Mordante
jloser
ldionne
Group Reviewers
Restricted Project
Summary

Poking buildkite out of an abundance of caution; but if buildkite likes this, I'm going to land it.

commit 99219e9f26f96bc814bb6caed3c4bdc7c8220fef (HEAD)
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Thu Sep 30 15:43:38 2021 -0400

    [libc++] Remove "// -*- C++ -*-" comments from all .cpp files.
    
    Even if these comments have a benefit in .h files (for editors that
    care about language but can't be configured to treat .h as C++ code),
    they certainly have no benefit for files with the .cpp extension.
    
    Discussed in D110794.

commit c0512f25ef61cf43ccbdfe6ac3202c131abfc93f
Author: Arthur O'Dwyer <arthur.j.odwyer@gmail.com>
Date:   Thu Sep 30 15:40:45 2021 -0400

    [libc++] [test] Remove "// -*- C++ -*-" comments from generated .cpp files.
    
    Even if these comments have a benefit in .h files (for editors that
    care about language but can't be configured to treat .h as C++ code),
    they certainly have no benefit for files with the .cpp extension.
    
    Discussed in D110794.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Sep 30 2021, 12:46 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 30 2021, 12:46 PM

I took a glance at some of the files and this looks good to me if CI passes. Good call on fixing the Python files that generate the headers to prevent this issue going forward.

jloser accepted this revision.Sep 30 2021, 12:55 PM
ldionne requested changes to this revision.Oct 1 2021, 1:28 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/version
1

Why this file?

This revision now requires changes to proceed.Oct 1 2021, 1:28 PM
ldionne accepted this revision.Oct 1 2021, 1:29 PM

Actually this LGTM if <version> keeps its C++ marker (but it can lose the -- version -- in the header comment).

Also, CI has been nowhere to be found for the past 36hrs and I don't know why yet. But this should be good to ship if your tests pass locally.

This revision is now accepted and ready to land.Oct 1 2021, 1:29 PM

Actually landed this this morning, as b82683b2eb3601f6e8970861b94ad7b37393aa90 + d4b59a05fc7507cf69993109443dc5af47ae4fa8 + c333505fa5d608ce21cb3887d0f49364e13a26df, and then 2a6b99d5f8231d96cb4cf03f86af0517743e8124 to revert the accidental change to <version>. I had edited both of the libcxx/utils/ scripts to remove the comment, and forgot that one of those two scripts didn't generate any .cpp files but did generate <version>.