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

[Nathan Sidwell via Phabricator via llvm-commits]

urnathan added inline comments.


================
Comment at: llvm/include/llvm/Demangle/ItaniumDemangle.h:3
-// See README.txt for instructions
-//===------------------------- ItaniumDemangle.h ----------------*- C++ -*-===//
 //
----------------
Quuxplusone wrote:
> 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. :)
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. 


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

https://reviews.llvm.org/D118135