[PATCH] D100714: [docs] Add a new tutorial that talk about how to make a change to llvm.

Mon May 17 11:26:02 PDT 2021

xgupta resigned from this revision.
xgupta added inline comments.

================
Comment at: llvm/docs/MyFirstTypoFix.rst:324
+   $ git checkout -b myfirstpatch
+   $ git commit -am "Clarify -Winfinite-recursion message"
+
----------------
dblaikie wrote:
> kuhnel wrote:
> > kuhnel wrote:
> > > dblaikie wrote:
> > > > mehdi_amini wrote:
> > > > > dblaikie wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > mehdi_amini wrote:
> > > > > > > > > xgupta wrote:
> > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > mehdi_amini wrote:
> > > > > > > > > > > > xgupta wrote:
> > > > > > > > > > > > > Beginners often forget to add a tag like for this patch Zhiqian didn't add [Docs] tag :)
> > > > > > > > > > > > > Should be mention here.
> > > > > > > > > > > > Is there a reference in an existing doc about the use of tags? I'm not sure we should tell beginners about things that aren't our documented norms.
> > > > > > > > > > > > (I don't use any tag other than "NFC")
> > > > > > > > > > > (I wouldn't see the need to add a tag to this very clear and explicit commit title for example)
> > > > > > > > > > So It depends on the person, LLVM Project is large with around 100 commits a day. I don't read the complete line of commit/patch. A tag is useful for me.
> > > > > > > > > > 
> > > > > > > > > > So if it is not documented, we can document it. Otherwise, I left this on other reviewers and patch author to decide what is best.
> > > > > > > > > Regardless, this seems to me to be out-of-line for a tutorial to define new conventions.
> > > > > > > > Eh, I disagree a bit here - it's the sort of feedback I'd give someone on the mailing list, so might as well write it down here - can be written down other places too.
> > > > > > > You're not clear with what you are disagreeing exactly: there is my point that a tutorial should not introduce new conventions, and whether in this case this is a documented convention or something that we've been consistently doing for a while.
> > > > > > > 
> > > > > > > We can talk about the latter, but I disagree that this is a settled thing which goes back to my former point: this isn't the revision to settle this.
> > > > > > > 
> > > > > > > (to expand a bit, even if this is out-of-scope for this revision I belive: this practice seems like a recent thing people have been doing, and it hasn't been introduced in a coordinated way and without clear guidelines as far as I know. In practice I see this as *noise* and I disagree that this a good use of the commit title limited space. (common guidelines are for git commit titles to be *short*))
> > > > > > > 
> > > > > > > 
> > > > > > Sorry, to be clear: I disagree that documenting the convention of using tags is inappropriate for this documentation at this time.
> > > > > > 
> > > > > > Tags have been used in commit descriptions from before the commit message was used in email titles (previously the title was a list of the files touched - so the title gave some sense of what part of the project the patch related to - albeit rather roughly/poorly) - but they're even more useful since that change (~8 years ago), eg: https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130128/163757.html
> > > > > > 
> > > > > > But sure, if you fundamentally object to it being done in this patch, so be it. 
> > > > > Yeah I fundamentally object to a tutorial introducing recommendation for this while there is no documentation and guidelines for it.
> > > > > If you want to propose a documentation for a guideline regarding this, why not, but please do as a separate patch and make sure it is documented outside of a tutorial.
> > > > My perspective is that this would be a fine place to include such a suggestion (I don't think as an explicit statement of what you must do, but as a suggestion of some things people do if you think that might clarify things, etc) - in the same way I would in mailing list usage (& have, several times - asking people to flag NFC changes as such as it makes it easier to review). I don't think everything in this document has to be (& I doubt it is, though I haven't read the whole thing) only things that are already formalized in other documentation. (for instance the heuristics mentioned under "Commit access" don't seem to be documented elsewhere - but I don't object to them either, I think it's reasonable to describe to newcomers some norms about how people generally interact in the community))
> > > Does someone have a (complete) list of tags to be used and their semantics?
> > Just looked at the commit history: https://github.com/llvm/llvm-project/commits/main
> > 
> > I can't really see a consistent set of tags people are using. Sometimes it's sub-prrojects (e.g. mlir, clangd) or architectures (ARM, x86) or features or change semantic (NFC). So I guess it would be hard to explain how to pick the right tag.
> > 
> > IMHO the only thing we can say here is that they exist and contributors should try to guess something reasonable...
> It's fairly informal - NFC, target (if it's in a target backend), specific optimization, or general library (ADT, Support, etc) or feature area (DebugInfo) are probably the broad categories.
> 
> But I'm not adamant this must be in the document - I'm a bit middle of the road. I don't think missing a tag is necessarily "forgetting" to do something that's required, nor that adding them is problematic or harmful either.
> 
> Seems easy enough to leave them out for now if they weren't added - or include it in the example commit message without necessarily any indicator that they must be included or a whole paragraph about them either.
> Is there a reference in an existing doc about the use of tags? I'm not sure we should tell beginners about things that aren't our documented norms.
> (I don't use any tag other than "NFC")

This [Tag] guideline is documented here: https://llvm.org/docs/DeveloperPolicy.html#commit-messages

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100714