[PATCH] D118135: [demangler] preserve line numbering in copied sources

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added inline comments.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:9-10
 //
 // 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.
 //
----------------
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.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:3
-// See README.txt for instructions
-//===------------------------- ItaniumDemangle.h ----------------*- C++ -*-===//
 //
----------------
ChuanqiXu wrote:
> urnathan wrote:
> > ChuanqiXu wrote:
> > > urnathan wrote:
> > > > ChuanqiXu wrote:
> > > > > Now the header name is missing, which is inconsistent with other headers. Is this intentional?
> > > > 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.]
> > > 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.
> > I suppose the 'See README' text could be the 2nd line?
> 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...
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. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118135/new/

https://reviews.llvm.org/D118135