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

Nathan Sidwell via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 26 04:06:40 PST 2022


urnathan added inline comments.


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


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

https://reviews.llvm.org/D118135



More information about the llvm-commits mailing list