[demangler] preserve line numbering in copied sources

[urnathan]

Summary

While my recent patch to prepend lines to the copied source files is functional, it disturbs the line numbering between the original and the copy. That makes development more awkward than necessary, as it is the copy that generally gets compiled first and emits compiler errors.

This uses sed to alter the first line, and also emits better emacs mode setting, getting both C++ mode and read-only mode.

[ChuanqiXu]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

Now the header name is missing, which is inconsistent with other headers. Is this intentional?

[urnathan]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

Yeah, I went back and forth on that. The 'Do not edit! really wants to be on the first line, and the emacs marker must be, to be effective. One's left with either

1. Omit the 'see README.txt' instruction. this is kind of useful to humans to explain why not edit
2. Omit the file name,
3. an overly-long line, yes, even though we have large displays, I'm still 80 column :)

I went with #2, we don't want this file manually edited anyway, is it important?

WDYT?

[as an aside, I don't really understand the desire for a file name here at all -- it's only the local filename, not the path from root, so doesn't really uniquify the file. But whatever.]

[ChuanqiXu]

@tstellar @dblaikie @Quuxplusone I want to ask if it is important to keep the filename in the file headers. I couldn't be sure about this. You guys looks experienced to answer the question than me.

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

In fact, I am not sure if the filename in the header is important or not. I would add some other more experienced folks to ask their opinion.

I agree that it is benefit to add a new line to tell human to not edit the file. But I don't think not every developer uses emacs (I use VSCode, for example). My point is that it is a little bit odd to do some change for a specific kind of users. I am not saying the work experience of emacs users is not important. I didn't say that.

I think another option would be add a blank line at the start of the original file and replace this line in the script.

[urnathan]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

I suppose the 'See README' text could be the 2nd line?

[ChuanqiXu]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

Yeah, I think so. The problems are:
(1) Keep the line number consistent after copied.
(2) Add information like "Do not edit" in the header to inform other programmers.
(3) We are not sure if it is good to delete the filename from the header.

I guess adding 1~2 lines in the front of the file might not be problem. But I am still not sure...

[urnathan]

Perhaps like this? the 2nd line indentation makes the 'do not edit' text stand out, I think?

[• Quuxplusone]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

FWIW, you can see libc++'s solution to this kind of thing in e.g. libcxx/test/libcxx/no_assert_include.compile.pass.cpp which is generated from libcxx/utils/generate_header_tests.py.

We don't include filenames (anymore) because they serve no purpose and bit-rot.
We don't include -*- C++ -*- in files whose names already end in .cpp, and we're inconsistent about doing so in files whose names end in .h (because it seems that the relevant editor programs already understand that .h files are C++, or at least C-that-might-as-well-be-highlighted-as-C++).
We don't encode any instructions to emacs, of course. I see no reason to do that. What, are you worried that someone might edit the file, commit their changes, and land the patch without testing, and btw they use emacs? (In this decade?) Coincidentally I just wrote https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/ — seems applicable here. :)

8–12

It would be useful for this comment to state explicitly, "The one in llvm gets automatically copied to libcxxabi. Don't edit the one in libcxxabi because your changes WILL be lost!" or vice versa, depending on what the situation is.
Also of course CI should diff the two files.

[dblaikie]

In D118135#3272068, @ChuanqiXu wrote:

@tstellar @dblaikie @Quuxplusone I want to ask if it is important to keep the filename in the file headers. I couldn't be sure about this. You guys looks experienced to answer the question than me.

General question about LLVM style, or are you wondering about a special consideration/exception for these special cloned files? I don't know of any especially important reason that they /must/ be there if there's particular value in not having them in this case... - but maybe other folks know more/different/have other feelings about it.

[urnathan]

llvm/include/llvm/Demangle/ItaniumDemangle.h

3

I'm not sure what you're asking for to change here. The file does include a filename -- are you asking for that to be removed? That seems an orthogonal issue.

There is already an emacs mode marker in the file, so what is the objection to extending that? Something stunningly close to what you describe actually happened -- changes were made to the llvm copy, tests were run, patch was reviewed and then committed. This was discovered when I continued my changes to the libcxxabi copy and blew away those llvm changes. It also seems perfectly apt to make files read-only (when possible), as one does want a jolt if you try and modify the wrong one, so I'm not buying your not-const argument here.

I've updated the README, which as you'll see already had a wish list about not cloning. I looked at that and also about adding a comparison test, but both seemed more complex than I have time for.

Please don't let perfect be the enemy of good.

[ChuanqiXu]

Given above discussion, I think the current one should be good.

libcxxabi/src/demangle/ItaniumDemangle.h

 //===------------------------- ItaniumDemangle.h ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// Generic itanium demangler library. This file has two byte-per-byte identical
-// copies in the source tree, one in libcxxabi, and the other in llvm.
+// Generic itanium demangler library.
+// There are two copies of this file in the source tree.  The one under
+// libcxxabi is the original and the one under llvm is the copy.  Use
+// cp-to-llvm.sh to update the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_ITANIUMDEMANGLE_H
 #define DEMANGLE_ITANIUMDEMANGLE_H
 
 // FIXME: (possibly) incomplete list of features that clang mangles that this
 // file does not yet support:

libcxxabi/src/demangle/README.txt

 Itanium Name Demangler Library
 ==============================
 
 Introduction
 ------------
 
-This directory contains the generic itanium name demangler library. The main
-purpose of the library is to demangle C++ symbols, i.e. convert the string
-"_Z1fv" into "f()". You can also use the CRTP base ManglingParser to perform
-some simple analysis on the mangled name, or (in LLVM) use the opaque
-ItaniumPartialDemangler to query the demangled AST.
+This directory contains the generic itanium name demangler
+library. The main purpose of the library is to demangle C++ symbols,
+i.e. convert the string "_Z1fv" into "f()". You can also use the CRTP
+base ManglingParser to perform some simple analysis on the mangled
+name, or (in LLVM) use the opaque ItaniumPartialDemangler to query the
+demangled AST.
 
 Why are there multiple copies of the this library in the source tree?
 ---------------------------------------------------------------------
 
-This directory is mirrored between libcxxabi/demangle and
-llvm/include/llvm/Demangle. The simple reason for this is that both projects
-need to demangle symbols, but neither can depend on each other. libcxxabi needs
-the demangler to implement __cxa_demangle, which is part of the itanium ABI
-spec. LLVM needs a copy for a bunch of places, but doesn't want to use the
-system's __cxa_demangle because it a) might not be available (i.e., on Windows),
-and b) probably isn't that up-to-date on the latest language features.
-
-The copy of the demangler in LLVM has some extra stuff that aren't needed in
-libcxxabi (ie, the MSVC demangler, ItaniumPartialDemangler), which depend on the
-shared generic components. Despite these differences, we want to keep the "core"
-generic demangling library identical between both copies to simplify development
-and testing.
-
-If you're working on the generic library, then do the work first in libcxxabi,
-then run the cp-to-llvm.sh script in src/demangle. This script takes as an
-argument the path to llvm, and re-copies the changes you made to libcxxabi over.
-Note that this script just blindly overwrites all changes to the generic library
-in llvm, so be careful.
-
-Because the core demangler needs to work in libcxxabi, everything needs to be
-declared in an anonymous namespace (see DEMANGLE_NAMESPACE_BEGIN), and you can't
-introduce any code that depends on the libcxx dylib.
-
-Hopefully, when LLVM becomes a monorepo, we can de-duplicate this code, and have
-both LLVM and libcxxabi depend on a shared demangler library.
+The canonical sources are in libcxxabi/src/demangle and some of the
+files are copied to llvm/include/llvm/Demangle.  The simple reason for
+this comes from before the monorepo, and both [sub]projects need to
+demangle symbols, but neither can depend on each other.
+
+* libcxxabi needs the demangler to implement __cxa_demangle, which is
+  part of the itanium ABI spec.
+
+* LLVM needs a copy for a bunch of places, and cannot rely on the
+  system's __cxa_demangle because it a) might not be available (i.e.,
+  on Windows), and b) may not be up-to-date on the latest language
+  features.
+
+The copy of the demangler in LLVM has some extra stuff that aren't
+needed in libcxxabi (ie, the MSVC demangler, ItaniumPartialDemangler),
+which depend on the shared generic components. Despite these
+differences, we want to keep the "core" generic demangling library
+identical between both copies to simplify development and testing.
+
+If you're working on the generic library, then do the work first in
+libcxxabi, then run the cp-to-llvm.sh script in src/demangle. This
+script takes as an optional argument the path to llvm, and copies the
+changes you made to libcxxabi over.  Note that this script just
+blindly overwrites all changes to the generic library in llvm, so be
+careful.
+
+Because the core demangler needs to work in libcxxabi, everything
+needs to be declared in an anonymous namespace (see
+DEMANGLE_NAMESPACE_BEGIN), and you can't introduce any code that
+depends on the libcxx dylib.
+
+FIXME: Now that LLVM is a monorepo, it should be possible to
+de-duplicate this code, and have both LLVM and libcxxabi depend on a
+shared demangler library.
 
 Testing
 -------
 
 The tests are split up between libcxxabi/test/{unit,}test_demangle.cpp, and
 llvm/unittest/Demangle. The llvm directory should only get tests for stuff not
 included in the core library. In the future though, we should probably move all
 the tests to LLVM.
 
 It is also a really good idea to run libFuzzer after non-trivial changes, see
 libcxxabi/fuzz/cxa_demangle_fuzzer.cpp and https://llvm.org/docs/LibFuzzer.html.

libcxxabi/src/demangle/StringView.h

 //===--- StringView.h -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // FIXME: Use std::string_view instead when we support C++17.
+// There are two copies of this file in the source tree.  The one under
+// libcxxabi is the original and the one under llvm is the copy.  Use
+// cp-to-llvm.sh to update the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_STRINGVIEW_H
 #define DEMANGLE_STRINGVIEW_H
 
 #include "DemangleConfig.h"
 #include <cassert>

libcxxabi/src/demangle/Utility.h

 //===--- Utility.h ----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// Provide some utility classes for use in the demangler(s).
+// Provide some utility classes for use in the demangler.
+// There are two copies of this file in the source tree.  The one in libcxxabi
+// is the original and the one in llvm is the copy.  Use cp-to-llvm.sh to update
+// the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_UTILITY_H
 #define DEMANGLE_UTILITY_H
 
 #include "StringView.h"
 #include <array>

libcxxabi/src/demangle/cp-to-llvm.sh

 read -p "This will overwrite the copies of $FILES in $LLVM_DEMANGLE_DIR; are you sure? [y/N]" -n 1 -r ANSWER
 echo
 
 if [[ $ANSWER =~ ^[Yy]$ ]]; then
     cp -f README.txt $LLVM_DEMANGLE_DIR
     chmod -w $LLVM_DEMANGLE_DIR/README.txt
     for I in $HDRS ; do
   rm -f $LLVM_DEMANGLE_DIR/$I
-  cat - $I >$LLVM_DEMANGLE_DIR/$I <<EOF
-// Do not edit! -*- read-only -*-
-// See README.txt for instructions
-EOF
+  dash=$(echo "$I---------------------------" | cut -c -27 |\
+       sed 's|[^-]*||')
+  sed -e '1s|^//=*-* .*\.h -*.*=*// *$|//===--- '"$I $dash"'-*- mode:c++;eval:(read-only-mode) -*-===//|' \
+      -e '2s|^// *$|//       Do not edit! See README.txt.|' \
+      $I >$LLVM_DEMANGLE_DIR/$I
   chmod -w $LLVM_DEMANGLE_DIR/$I
     done
 fi

llvm/include/llvm/Demangle/ItaniumDemangle.h

-// Do not edit! -*- read-only -*-
-// See README.txt for instructions
-//===------------------------- ItaniumDemangle.h ----------------*- C++ -*-===//
-//
+//===--- ItaniumDemangle.h -----------*- mode:c++;eval:(read-only-mode) -*-===//
+//       Do not edit! See README.txt.
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// Generic itanium demangler library. This file has two byte-per-byte identical
-// copies in the source tree, one in libcxxabi, and the other in llvm.
+// Generic itanium demangler library.
+// There are two copies of this file in the source tree.  The one under
+// libcxxabi is the original and the one under llvm is the copy.  Use
+// cp-to-llvm.sh to update the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_ITANIUMDEMANGLE_H
 #define DEMANGLE_ITANIUMDEMANGLE_H
 
 // FIXME: (possibly) incomplete list of features that clang mangles that this
 // file does not yet support:

llvm/include/llvm/Demangle/README.txt

 Itanium Name Demangler Library
 ==============================
 
 Introduction
 ------------
 
-This directory contains the generic itanium name demangler library. The main
-purpose of the library is to demangle C++ symbols, i.e. convert the string
-"_Z1fv" into "f()". You can also use the CRTP base ManglingParser to perform
-some simple analysis on the mangled name, or (in LLVM) use the opaque
-ItaniumPartialDemangler to query the demangled AST.
+This directory contains the generic itanium name demangler
+library. The main purpose of the library is to demangle C++ symbols,
+i.e. convert the string "_Z1fv" into "f()". You can also use the CRTP
+base ManglingParser to perform some simple analysis on the mangled
+name, or (in LLVM) use the opaque ItaniumPartialDemangler to query the
+demangled AST.
 
 Why are there multiple copies of the this library in the source tree?
 ---------------------------------------------------------------------
 
-This directory is mirrored between libcxxabi/demangle and
-llvm/include/llvm/Demangle. The simple reason for this is that both projects
-need to demangle symbols, but neither can depend on each other. libcxxabi needs
-the demangler to implement __cxa_demangle, which is part of the itanium ABI
-spec. LLVM needs a copy for a bunch of places, but doesn't want to use the
-system's __cxa_demangle because it a) might not be available (i.e., on Windows),
-and b) probably isn't that up-to-date on the latest language features.
-
-The copy of the demangler in LLVM has some extra stuff that aren't needed in
-libcxxabi (ie, the MSVC demangler, ItaniumPartialDemangler), which depend on the
-shared generic components. Despite these differences, we want to keep the "core"
-generic demangling library identical between both copies to simplify development
-and testing.
-
-If you're working on the generic library, then do the work first in libcxxabi,
-then run the cp-to-llvm.sh script in src/demangle. This script takes as an
-argument the path to llvm, and re-copies the changes you made to libcxxabi over.
-Note that this script just blindly overwrites all changes to the generic library
-in llvm, so be careful.
-
-Because the core demangler needs to work in libcxxabi, everything needs to be
-declared in an anonymous namespace (see DEMANGLE_NAMESPACE_BEGIN), and you can't
-introduce any code that depends on the libcxx dylib.
-
-Hopefully, when LLVM becomes a monorepo, we can de-duplicate this code, and have
-both LLVM and libcxxabi depend on a shared demangler library.
+The canonical sources are in libcxxabi/src/demangle and some of the
+files are copied to llvm/include/llvm/Demangle.  The simple reason for
+this comes from before the monorepo, and both [sub]projects need to
+demangle symbols, but neither can depend on each other.
+
+* libcxxabi needs the demangler to implement __cxa_demangle, which is
+  part of the itanium ABI spec.
+
+* LLVM needs a copy for a bunch of places, and cannot rely on the
+  system's __cxa_demangle because it a) might not be available (i.e.,
+  on Windows), and b) may not be up-to-date on the latest language
+  features.
+
+The copy of the demangler in LLVM has some extra stuff that aren't
+needed in libcxxabi (ie, the MSVC demangler, ItaniumPartialDemangler),
+which depend on the shared generic components. Despite these
+differences, we want to keep the "core" generic demangling library
+identical between both copies to simplify development and testing.
+
+If you're working on the generic library, then do the work first in
+libcxxabi, then run the cp-to-llvm.sh script in src/demangle. This
+script takes as an optional argument the path to llvm, and copies the
+changes you made to libcxxabi over.  Note that this script just
+blindly overwrites all changes to the generic library in llvm, so be
+careful.
+
+Because the core demangler needs to work in libcxxabi, everything
+needs to be declared in an anonymous namespace (see
+DEMANGLE_NAMESPACE_BEGIN), and you can't introduce any code that
+depends on the libcxx dylib.
+
+FIXME: Now that LLVM is a monorepo, it should be possible to
+de-duplicate this code, and have both LLVM and libcxxabi depend on a
+shared demangler library.
 
 Testing
 -------
 
 The tests are split up between libcxxabi/test/{unit,}test_demangle.cpp, and
 llvm/unittest/Demangle. The llvm directory should only get tests for stuff not
 included in the core library. In the future though, we should probably move all
 the tests to LLVM.
 
 It is also a really good idea to run libFuzzer after non-trivial changes, see
 libcxxabi/fuzz/cxa_demangle_fuzzer.cpp and https://llvm.org/docs/LibFuzzer.html.

llvm/include/llvm/Demangle/StringView.h

-// Do not edit! -*- read-only -*-
-// See README.txt for instructions
-//===--- StringView.h -------------------------------------------*- C++ -*-===//
-//
+//===--- StringView.h ----------------*- mode:c++;eval:(read-only-mode) -*-===//
+//       Do not edit! See README.txt.
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // FIXME: Use std::string_view instead when we support C++17.
+// There are two copies of this file in the source tree.  The one under
+// libcxxabi is the original and the one under llvm is the copy.  Use
+// cp-to-llvm.sh to update the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_STRINGVIEW_H
 #define DEMANGLE_STRINGVIEW_H
 
 #include "DemangleConfig.h"
 #include <cassert>

llvm/include/llvm/Demangle/Utility.h

-// Do not edit! -*- read-only -*-
-// See README.txt for instructions
-//===--- Utility.h ----------------------------------------------*- C++ -*-===//
-//
+//===--- Utility.h -------------------*- mode:c++;eval:(read-only-mode) -*-===//
+//       Do not edit! See README.txt.
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
-// Provide some utility classes for use in the demangler(s).
+// Provide some utility classes for use in the demangler.
+// There are two copies of this file in the source tree.  The one in libcxxabi
+// is the original and the one in llvm is the copy.  Use cp-to-llvm.sh to update
+// the copy.  See README.txt for more details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef DEMANGLE_UTILITY_H
 #define DEMANGLE_UTILITY_H
 
 #include "StringView.h"
 #include <array>