[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 2 inline comments as done.
aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

reames wrote:
> aaron.ballman wrote:
> > reames wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > reames wrote:
> > > > > > Removing this item seems very off topic for the change description, 
> > > > > > and certainly hasn't been discussed in the linked thread.  Please 
> > > > > > add this back in a separate commit.
> > > > > > 
> > > > > > (To be clear, no objections to the overall change, just the removal 
> > > > > > of the phab link text.)
> > > > > Hmm, I thought this was obsoleted by the new text (it is covered by 
> > > > > "other kinds of metadata"). That said, losing that link is definitely 
> > > > > a regression, so thank you for pointing this out! I'll find a way to 
> > > > > add it back in (either as a stand-alone bullet point or incorporated 
> > > > > into the new text).
> > > > I restored the link in 
> > > > https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
> > > >  as part of the new bullet; please let me know if you have additional 
> > > > concerns.
> > > That 90% covers it.  What's left is some minor framing.  I'd suggest 
> > > separating that point into two.  The first should be recommended metadata 
> > > (phab, issues link), and the second can be the additional metadata point. 
> > >  Something like:
> > > 
> > > ```
> > > If the patch has been reviewed, add a link to its review page, as shown
> > >   `here 
> > > `_. If 
> > > the patch fixes a bug in GitHub Issues, we encourage adding a reference 
> > > to the issue being closed, as described `here 
> > > `_.
> > > 
> > > It is also acceptable to add other metadata to the commit message to 
> > > automate processes, including for downstream consumers. and including 
> > > links to resources that are not available to the entire community. 
> > > However, such links and/or metadata should not be used in place of making 
> > > the commit message self-explanatory.  
> > > 
> > > ```
> > > All of the above is just reorganizing what you had written with some very 
> > > minor copy editing.  I'd separately suggest adding the following sentence 
> > > at the end of the second bullet.
> > > 
> > > Note that such non-public links are *only* allowed in commit messages, 
> > > and should not be included in the submitted code.  
> > I did some minor rewording for clarity, so how about:
> > ```
> > * If the patch has been reviewed, add a link to its review page, as shown
> >   `here `__.
> >   If the patch fixes a bug in GitHub Issues, we encourage adding a 
> > reference to
> >   the issue being closed, as described
> >   `here `__.
> > 
> > * It is also acceptable to add other metadata to the commit message to 
> > automate
> >   processes, including for downstream consumers. This metadata can include
> >   links to resources that are not available to the entire community. 
> > However,
> >   such links and/or metadata should not be used in place of making the 
> > commit
> >   message self-explanatory.
> > ```
> > 
> > > All of the above is just reorganizing what you had written with some very 
> > > minor copy editing. I'd separately suggest adding the following sentence 
> > > at the end of the second bullet.
> > > 
> > > Note that such non-public links are *only* allowed in commit messages, 
> > > and should not be included in the submitted code.
> > 
> > I think this might need more wordsmithing, which is why I left out of the 
> > simple reorganization. The non-public links aren't limited to commit 
> > messages -- for example, they're fine to use in a phabricator review or 
> > github issue comment, etc. So I don't want to be too restrictive with the 
> > wording, though I agree with the intent. How about something along the 
> > lines of:
> > 
> > Note that such non-public links should not be included in the submitted 
> > code.
> LGTM to both parts.  Wording is hard, and good catch on the second.  :)
Thank you for the help! Because this is rearranging existing text and the 
additional text is a minor point of clarity, I'll land those changes shortly -- 
but as always, if folks have more post-commit review feedback on those changes, 
please speak up and I'll work with you on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

aaron.ballman wrote:
> reames wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > reames wrote:
> > > > > Removing this item seems very off topic for the change description, 
> > > > > and certainly hasn't been discussed in the linked thread.  Please add 
> > > > > this back in a separate commit.
> > > > > 
> > > > > (To be clear, no objections to the overall change, just the removal 
> > > > > of the phab link text.)
> > > > Hmm, I thought this was obsoleted by the new text (it is covered by 
> > > > "other kinds of metadata"). That said, losing that link is definitely a 
> > > > regression, so thank you for pointing this out! I'll find a way to add 
> > > > it back in (either as a stand-alone bullet point or incorporated into 
> > > > the new text).
> > > I restored the link in 
> > > https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
> > >  as part of the new bullet; please let me know if you have additional 
> > > concerns.
> > That 90% covers it.  What's left is some minor framing.  I'd suggest 
> > separating that point into two.  The first should be recommended metadata 
> > (phab, issues link), and the second can be the additional metadata point.  
> > Something like:
> > 
> > ```
> > If the patch has been reviewed, add a link to its review page, as shown
> >   `here `_. 
> > If the patch fixes a bug in GitHub Issues, we encourage adding a reference 
> > to the issue being closed, as described `here 
> > `_.
> > 
> > It is also acceptable to add other metadata to the commit message to 
> > automate processes, including for downstream consumers. and including links 
> > to resources that are not available to the entire community. However, such 
> > links and/or metadata should not be used in place of making the commit 
> > message self-explanatory.  
> > 
> > ```
> > All of the above is just reorganizing what you had written with some very 
> > minor copy editing.  I'd separately suggest adding the following sentence 
> > at the end of the second bullet.
> > 
> > Note that such non-public links are *only* allowed in commit messages, and 
> > should not be included in the submitted code.  
> I did some minor rewording for clarity, so how about:
> ```
> * If the patch has been reviewed, add a link to its review page, as shown
>   `here `__.
>   If the patch fixes a bug in GitHub Issues, we encourage adding a reference 
> to
>   the issue being closed, as described
>   `here `__.
> 
> * It is also acceptable to add other metadata to the commit message to 
> automate
>   processes, including for downstream consumers. This metadata can include
>   links to resources that are not available to the entire community. However,
>   such links and/or metadata should not be used in place of making the commit
>   message self-explanatory.
> ```
> 
> > All of the above is just reorganizing what you had written with some very 
> > minor copy editing. I'd separately suggest adding the following sentence at 
> > the end of the second bullet.
> > 
> > Note that such non-public links are *only* allowed in commit messages, and 
> > should not be included in the submitted code.
> 
> I think this might need more wordsmithing, which is why I left out of the 
> simple reorganization. The non-public links aren't limited to commit messages 
> -- for example, they're fine to use in a phabricator review or github issue 
> comment, etc. So I don't want to be too restrictive with the wording, though 
> I agree with the intent. How about something along the lines of:
> 
> Note that such non-public links should not be included in the submitted code.
LGTM to both parts.  Wording is hard, and good catch on the second.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

reames wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > reames wrote:
> > > > Removing this item seems very off topic for the change description, and 
> > > > certainly hasn't been discussed in the linked thread.  Please add this 
> > > > back in a separate commit.
> > > > 
> > > > (To be clear, no objections to the overall change, just the removal of 
> > > > the phab link text.)
> > > Hmm, I thought this was obsoleted by the new text (it is covered by 
> > > "other kinds of metadata"). That said, losing that link is definitely a 
> > > regression, so thank you for pointing this out! I'll find a way to add it 
> > > back in (either as a stand-alone bullet point or incorporated into the 
> > > new text).
> > I restored the link in 
> > https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
> >  as part of the new bullet; please let me know if you have additional 
> > concerns.
> That 90% covers it.  What's left is some minor framing.  I'd suggest 
> separating that point into two.  The first should be recommended metadata 
> (phab, issues link), and the second can be the additional metadata point.  
> Something like:
> 
> ```
> If the patch has been reviewed, add a link to its review page, as shown
>   `here `_. 
> If the patch fixes a bug in GitHub Issues, we encourage adding a reference to 
> the issue being closed, as described `here 
> `_.
> 
> It is also acceptable to add other metadata to the commit message to automate 
> processes, including for downstream consumers. and including links to 
> resources that are not available to the entire community. However, such links 
> and/or metadata should not be used in place of making the commit message 
> self-explanatory.  
> 
> ```
> All of the above is just reorganizing what you had written with some very 
> minor copy editing.  I'd separately suggest adding the following sentence at 
> the end of the second bullet.
> 
> Note that such non-public links are *only* allowed in commit messages, and 
> should not be included in the submitted code.  
I did some minor rewording for clarity, so how about:
```
* If the patch has been reviewed, add a link to its review page, as shown
  `here `__.
  If the patch fixes a bug in GitHub Issues, we encourage adding a reference to
  the issue being closed, as described
  `here `__.

* It is also acceptable to add other metadata to the commit message to automate
  processes, including for downstream consumers. This metadata can include
  links to resources that are not available to the entire community. However,
  such links and/or metadata should not be used in place of making the commit
  message self-explanatory.
```

> All of the above is just reorganizing what you had written with some very 
> minor copy editing. I'd separately suggest adding the following sentence at 
> the end of the second bullet.
> 
> Note that such non-public links are *only* allowed in commit messages, and 
> should not be included in the submitted code.

I think this might need more wordsmithing, which is why I left out of the 
simple reorganization. The non-public links aren't limited to commit messages 
-- for example, they're fine to use in a phabricator review or github issue 
comment, etc. So I don't want to be too restrictive with the wording, though I 
agree with the intent. How about something along the lines of:

Note that such non-public links should not be included in the submitted code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

aaron.ballman wrote:
> aaron.ballman wrote:
> > reames wrote:
> > > Removing this item seems very off topic for the change description, and 
> > > certainly hasn't been discussed in the linked thread.  Please add this 
> > > back in a separate commit.
> > > 
> > > (To be clear, no objections to the overall change, just the removal of 
> > > the phab link text.)
> > Hmm, I thought this was obsoleted by the new text (it is covered by "other 
> > kinds of metadata"). That said, losing that link is definitely a 
> > regression, so thank you for pointing this out! I'll find a way to add it 
> > back in (either as a stand-alone bullet point or incorporated into the new 
> > text).
> I restored the link in 
> https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
>  as part of the new bullet; please let me know if you have additional 
> concerns.
That 90% covers it.  What's left is some minor framing.  I'd suggest separating 
that point into two.  The first should be recommended metadata (phab, issues 
link), and the second can be the additional metadata point.  Something like:

```
If the patch has been reviewed, add a link to its review page, as shown
  `here `_. If 
the patch fixes a bug in GitHub Issues, we encourage adding a reference to the 
issue being closed, as described `here 
`_.

It is also acceptable to add other metadata to the commit message to automate 
processes, including for downstream consumers. and including links to resources 
that are not available to the entire community. However, such links and/or 
metadata should not be used in place of making the commit message 
self-explanatory.  

```
All of the above is just reorganizing what you had written with some very minor 
copy editing.  I'd separately suggest adding the following sentence at the end 
of the second bullet.

Note that such non-public links are *only* allowed in commit messages, and 
should not be included in the submitted code.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

aaron.ballman wrote:
> reames wrote:
> > Removing this item seems very off topic for the change description, and 
> > certainly hasn't been discussed in the linked thread.  Please add this back 
> > in a separate commit.
> > 
> > (To be clear, no objections to the overall change, just the removal of the 
> > phab link text.)
> Hmm, I thought this was obsoleted by the new text (it is covered by "other 
> kinds of metadata"). That said, losing that link is definitely a regression, 
> so thank you for pointing this out! I'll find a way to add it back in (either 
> as a stand-alone bullet point or incorporated into the new text).
I restored the link in 
https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d
 as part of the new bullet; please let me know if you have additional concerns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

reames wrote:
> Removing this item seems very off topic for the change description, and 
> certainly hasn't been discussed in the linked thread.  Please add this back 
> in a separate commit.
> 
> (To be clear, no objections to the overall change, just the removal of the 
> phab link text.)
Hmm, I thought this was obsoleted by the new text (it is covered by "other 
kinds of metadata"). That said, losing that link is definitely a regression, so 
thank you for pointing this out! I'll find a way to add it back in (either as a 
stand-alone bullet point or incorporated into the new text).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Philip Reames via Phabricator via cfe-commits]

reames added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:349
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.

Removing this item seems very off topic for the change description, and 
certainly hasn't been discussed in the linked thread.  Please add this back in 
a separate commit.

(To be clear, no objections to the overall change, just the removal of the phab 
link text.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG677326999f27: Specify the developer policy around links to 
external resources (authored by aaron.ballman).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small 
feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the 
message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,13 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub 
Issues,
+  we encourage adding a reference to the issue being closed, as described
+  `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,13 @@
   related commit. This could be as simple as "Revert commit 

[PATCH] D155081: Specify the developer policy around links to external resources

[Chris Lattner via Phabricator via cfe-commits]

lattner accepted this revision.
lattner added a comment.
This revision is now accepted and ready to land.

Looks like a great improvement to the policy, thank you all for sorting through 
this!


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Tanya Lattner via Phabricator via cfe-commits]

tonic added a comment.

@lattner Please review.


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 549979.
aaron.ballman added a comment.

Tweaked wording to link to the policy about adding links to github issues in 
commit messages instead.


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

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small 
feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the 
message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,13 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
-  `here `_.
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub 
Issues,
+  we encourage adding a reference to the issue being closed, as described
+  `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,13 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has 

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

rZhBoYao wrote:
> probinson wrote:
> > mehdi_amini wrote:
> > > ldionne wrote:
> > > > smeenai wrote:
> > > > > aaron.ballman wrote:
> > > > > > arsenm wrote:
> > > > > > > I haven't quite figured out what the exact syntaxes which are 
> > > > > > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > > > > > Yup, it does support that form as well. I had heard more than once 
> > > > > > during code review that folks seem to prefer the full link because 
> > > > > > it's easier to click on that from the commit message than it is to 
> > > > > > navigate to the fix from the number alone. That seemed like a 
> > > > > > pretty good reason to recommend the full form, but I don't have 
> > > > > > strong opinions.
> > > > > +1 for encouraging the full link
> > > > Perhaps we could encourage using `https://llvm.org/PR12345` instead? 
> > > > Does anybody know whether `llvm.org/PRXXX` is something that we intend 
> > > > to keep around with the Github transition or not?
> > > @arsenm: It's documented 
> > > https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
> > > And for linking cross-repo: 
> > > https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests
> > > Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> > > anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> > > around with the Github transition or not?
> > 
> > Currently the PRxxx links are to the old bugzillas, not the Github issues. 
> > It might be sad to lose that.
> If the full link is preferred, can you update the first bullet point in [[ 
> https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs | the 
> Resolving/Closing bugs section of LLVM Bug Life Cycle ]]?
Given that the RFC was specifically about links and not about bug lifecycle, 
I'd rather change that policy in a different patch (I suspect someone would 
have to make a full RFC to make the change; I don't feel strongly enough to go 
through that process myself). It might make more sense to link to that section 
of the documentation from here instead of spelling out something that may sound 
like a conflicting policy. e.g.,
```
If the patch fixes a bug in GitHub Issues, we encourage adding a reference to 
the issue being closed, as described `here 
`_.
```


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

This looks good to me.


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[PoYao Chang via Phabricator via cfe-commits]

rZhBoYao added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

probinson wrote:
> mehdi_amini wrote:
> > ldionne wrote:
> > > smeenai wrote:
> > > > aaron.ballman wrote:
> > > > > arsenm wrote:
> > > > > > I haven't quite figured out what the exact syntaxes which are 
> > > > > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > > > > Yup, it does support that form as well. I had heard more than once 
> > > > > during code review that folks seem to prefer the full link because 
> > > > > it's easier to click on that from the commit message than it is to 
> > > > > navigate to the fix from the number alone. That seemed like a pretty 
> > > > > good reason to recommend the full form, but I don't have strong 
> > > > > opinions.
> > > > +1 for encouraging the full link
> > > Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> > > anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> > > around with the Github transition or not?
> > @arsenm: It's documented 
> > https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
> > And for linking cross-repo: 
> > https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests
> > Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> > anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> > around with the Github transition or not?
> 
> Currently the PRxxx links are to the old bugzillas, not the Github issues. It 
> might be sad to lose that.
If the full link is preferred, can you update the first bullet point in [[ 
https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs | the 
Resolving/Closing bugs section of LLVM Bug Life Cycle ]]?


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 549370.
aaron.ballman marked an inline comment as done.
aaron.ballman added a comment.

Updated based on review feedback.


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

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small 
feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the 
message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,16 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub 
Issues,
+  we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a
+  link to its review page, as shown
   `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files point to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,16 @@
   related commit. This could be as simple as "Revert commit  

[PATCH] D155081: Specify the developer policy around links to external resources

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin accepted this revision.
cor3ntin added a comment.

This seems very reasonable to me




Comment at: llvm/docs/DeveloperPolicy.rst:264
 
+#. Ensure that links in source code and test files are to publicly available
+   resources and are used primarily to add additional information rather than




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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 549369.
aaron.ballman added a comment.

Accidentally uploaded a partial diff instead of the full set of changes.


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

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small 
feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files are to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the 
message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,16 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub 
Issues,
+  we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a
+  link to its review page, as shown
   `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files are to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,16 @@
   related commit. This could be as simple as "Revert commit  because it
   

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155081#4507579 , @tonic wrote:

> As I mentioned on the Discourse thread, but if this policy change is made, 
> there should be some discussion about if this applies retroactively. Making a 
> note here, because it probably should be included in the changes to Developer 
> Policy if the radar links are not removed but not allowed going forward.

I believe this was sufficiently clarified on the Discourse thread -- this 
policy follows all the same procedures as our other coding style policies. If 
you think the existing policy is unclear in this area, I think that should be 
handled as a separate policy discussion instead of as an ad hoc statement for 
just this change.


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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 549365.
aaron.ballman marked 2 inline comments as done.
aaron.ballman added a comment.

Updated based on review comments.


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

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -354,14 +354,16 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* It is acceptable to add metadata to the commit message to automate processes.
-  If the patch fixes a bug in GitHub Issues, we encourage adding
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub 
Issues,
+  we encourage adding
   "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
   the issue in GitHub. If the patch has been reviewed, we encourage adding a
   link to its review page, as shown
   `here `_.
   Other kinds of metadata are also acceptable, including links to resources
-  that are not available to the entire community.
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -354,14 +354,16 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* It is acceptable to add metadata to the commit message to automate processes.
-  If the patch fixes a bug in GitHub Issues, we encourage adding
+* It is acceptable to add metadata to the commit message to automate processes,
+  including for downstream consumers. If the patch fixes a bug in GitHub Issues,
+  we encourage adding
   "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
   the issue in GitHub. If the patch has been reviewed, we encourage adding a
   link to its review page, as shown
   `here `_.
   Other kinds of metadata are also acceptable, including links to resources
-  that are not available to the entire community.
+  that are not available to the entire community. However, such links should
+  not be used in place of making the commit message self-explanatory.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

mehdi_amini wrote:
> ldionne wrote:
> > smeenai wrote:
> > > aaron.ballman wrote:
> > > > arsenm wrote:
> > > > > I haven't quite figured out what the exact syntaxes which are 
> > > > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > > > Yup, it does support that form as well. I had heard more than once 
> > > > during code review that folks seem to prefer the full link because it's 
> > > > easier to click on that from the commit message than it is to navigate 
> > > > to the fix from the number alone. That seemed like a pretty good reason 
> > > > to recommend the full form, but I don't have strong opinions.
> > > +1 for encouraging the full link
> > Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> > anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> > around with the Github transition or not?
> @arsenm: It's documented 
> https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
> And for linking cross-repo: 
> https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests
> Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> around with the Github transition or not?

Currently the PRxxx links are to the old bugzillas, not the Github issues. It 
might be sad to lose that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Tanya Lattner via Phabricator via cfe-commits]

tonic added a comment.

As I mentioned on the Discourse thread, but if this policy change is made, 
there should be some discussion about if this applies retroactively. Making a 
note here, because it probably should be included in the changes to Developer 
Policy if the radar links are not removed but not allowed going forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D155081#4499046 , @lattner wrote:

> Thank you so much for raising this issue Aaron.
>
> Documenting the policy in the developer policy is totally the right thing to 
> do given this cross-cuts individual subprojects, but I don't think we have 
> consensus on what the right approach is.  This is a pretty important issue to 
> a number of LLVM contributors, and I think we should have a first-principles 
> discussion about it on the forum before instituting a policy.
> https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847/43?u=clattner

You're linking to that very discussion; I've not seen a whole lot of new input 
on that thread which did seem to come to a consensus, but I'm happy to wait 
longer for folks to weigh in before proceeding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Chris Lattner via Phabricator via cfe-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

Thank you so much for raising this issue Aaron.

Documenting the policy in the developer policy is totally the right thing to do 
given this cross-cuts individual subprojects, but I don't think we have 
consensus on what the right approach is.  This is a pretty important issue to a 
number of LLVM contributors, and I think we should have a first-principles 
discussion about it on the forum before instituting a policy.
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847/43?u=clattner


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

Approving in support, but don't consider this enough as-is. I suspect you 
should get @lattner approval here :)




Comment at: llvm/docs/DeveloperPolicy.rst:357
 
-* If the patch has been reviewed, add a link to its review page, as shown
+* It is acceptable to add metadata to the commit message to automate processes.
+  If the patch fixes a bug in GitHub Issues, we encourage adding

More explicitly: "automate processes, including for downstream consumers"?



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

ldionne wrote:
> smeenai wrote:
> > aaron.ballman wrote:
> > > arsenm wrote:
> > > > I haven't quite figured out what the exact syntaxes which are 
> > > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > > Yup, it does support that form as well. I had heard more than once during 
> > > code review that folks seem to prefer the full link because it's easier 
> > > to click on that from the commit message than it is to navigate to the 
> > > fix from the number alone. That seemed like a pretty good reason to 
> > > recommend the full form, but I don't have strong opinions.
> > +1 for encouraging the full link
> Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
> anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
> around with the Github transition or not?
@arsenm: It's documented 
https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword
And for linking cross-repo: 
https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

smeenai wrote:
> aaron.ballman wrote:
> > arsenm wrote:
> > > I haven't quite figured out what the exact syntaxes which are 
> > > automatically recognized. It seems to recognize "Fixes #Nxyz"
> > Yup, it does support that form as well. I had heard more than once during 
> > code review that folks seem to prefer the full link because it's easier to 
> > click on that from the commit message than it is to navigate to the fix 
> > from the number alone. That seemed like a pretty good reason to recommend 
> > the full form, but I don't have strong opinions.
> +1 for encouraging the full link
Perhaps we could encourage using `https://llvm.org/PR12345` instead? Does 
anybody know whether `llvm.org/PRXXX` is something that we intend to keep 
around with the Github transition or not?



Comment at: llvm/docs/DeveloperPolicy.rst:363-364
   `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community.
 

Do you think this adds a useful reminder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

aaron.ballman wrote:
> arsenm wrote:
> > I haven't quite figured out what the exact syntaxes which are automatically 
> > recognized. It seems to recognize "Fixes #Nxyz"
> Yup, it does support that form as well. I had heard more than once during 
> code review that folks seem to prefer the full link because it's easier to 
> click on that from the commit message than it is to navigate to the fix from 
> the number alone. That seemed like a pretty good reason to recommend the full 
> form, but I don't have strong opinions.
+1 for encouraging the full link


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

arsenm wrote:
> I haven't quite figured out what the exact syntaxes which are automatically 
> recognized. It seems to recognize "Fixes #Nxyz"
Yup, it does support that form as well. I had heard more than once during code 
review that folks seem to prefer the full link because it's easier to click on 
that from the commit message than it is to navigate to the fix from the number 
alone. That seemed like a pretty good reason to recommend the full form, but I 
don't have strong opinions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: llvm/docs/DeveloperPolicy.rst:359
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a

I haven't quite figured out what the exact syntaxes which are automatically 
recognized. It seems to recognize "Fixes #Nxyz"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155081

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rnk, lattner, doug.gregor, probinson, ldionne, 
arsenm, mehdi_amini.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.

This specifies the developer policy on adding links in source & test files and 
commit messages, related to discussion at: 
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847

The intent is to discourage adding links to resources that are not available to 
the community as a whole (dead links, links to internal documentation, links to 
internal bug trackers, etc) from source and test files, while still allowing 
such links to appear in other contexts as needed. It suggests to instead add 
sufficient context in the surrounding comments to make such links unnecessary.

It also clarifies that these links can appear in commit messages as metadata 
(similar to how we already have Differential Revision and Fixes metadata with 
links).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155081

Files:
  llvm/docs/DeveloperPolicy.rst


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small 
feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files are to publicly available
+   resources and are used primarily to add additional information rather than
+   to supply critical context. The surrounding comments should be sufficient
+   to provide the context behind such links.
+
 Additionally, the committer is responsible for addressing any problems found in
 the future that the change is responsible for.  For example:
 
@@ -336,8 +346,6 @@
   code snippets and gory details should be left to bug comments, web
   review or the mailing list.
 
-* If the patch fixes a bug in GitHub Issues, please include the PR# in the 
message.
-
 * Text formatting and spelling should follow the same rules as documentation
   and in-code comments, ex. capitalization, full stop, etc.
 
@@ -346,8 +354,14 @@
   related commit. This could be as simple as "Revert commit  because it
   caused PR#".
 
-* If the patch has been reviewed, add a link to its review page, as shown
+* It is acceptable to add metadata to the commit message to automate processes.
+  If the patch fixes a bug in GitHub Issues, we encourage adding
+  "Fixes https://github.com/llvm/llvm-project/issues/12345; to automate closing
+  the issue in GitHub. If the patch has been reviewed, we encourage adding a
+  link to its review page, as shown
   `here `_.
+  Other kinds of metadata are also acceptable, including links to resources
+  that are not available to the entire community.
 
 For minor violations of these recommendations, the community normally favors
 reminding the contributor of this policy over reverting. Minor corrections and


Index: llvm/docs/DeveloperPolicy.rst
===
--- llvm/docs/DeveloperPolicy.rst
+++ llvm/docs/DeveloperPolicy.rst
@@ -201,6 +201,11 @@
   entire failing program into ``llvm/test`` as this creates a *time-to-test*
   burden on all developers. Please keep them short.
 
+* Avoid adding links to resources that are not available to the entire
+  community, such as links to private bug trackers, internal corporate
+  documentation, etc. Instead, add sufficient comments to the test to provide
+  the context behind such links.
+
 Note that llvm/test and clang/test are designed for regression and small feature
 tests only. More extensive test cases (e.g., entire applications, benchmarks,
 etc) should be added to the ``llvm-test`` test suite.  The llvm-test suite is
@@ -256,6 +261,11 @@
the change (more invasive changes require more testing). A reasonable subset
might be something like "``llvm-test/MultiSource/Benchmarks``".
 
+#. Ensure that links in source code and test files are to publicly available
+   resources and are used primarily to add additional information