[PATCH] D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator.

Wed Jan 9 12:52:59 PST 2019

kristina added a comment.

In D56482#1351638 <https://reviews.llvm.org/D56482#1351638>, @MyDeveloperDay wrote:

> > ! In D56482#1350874 <https://reviews.llvm.org/D56482#1350874>, @kristina wrote:
> > 
> > I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.
>
> Sorry I wasn't clear, I'm using a script from D55523: [clang-tidy] Linting .rst documentation <https://reviews.llvm.org/D55523> to validate .rst files, if you read the revision there is some info on the rational. (I simply ran it here on a copy of the file you are editing prior to your revision). You'll see from D55523 <https://reviews.llvm.org/D55523> that I mention that I saw a lot of review comments which amounted to stylistic changes in the .rst files, and yet I see a lot of reviewers time pointing these out, time and time again and its all simple stuff that could be automated. (or at least added to such a how to guide)
>
> it would be good if we could ensure .rst files are checked prior to review (how we do this I don't know) without initially generating 10,000 of minor whitespaces changes.
>
> when I reviewed all .rst files currently checked into LLVM I found 10,000's of simple violations (maybe people aren't worried about it but certainly it comes up a lot in reviews and causes the second or third revisions)
>
> I think another point is that lot of review comments could potentially be caught by clang-tidy, there is sometimes lots of the review conversations  especially with new contributors, about "don't use anonymous namspaces but prefer static functions","clang-format the file", don't overly use auto", don't have "const value types" all stuff that is in the CodingStyle but is easily missed when making or updating a review.
>
> It would be good if we had a mechanism to be able to validate a patch prior to commit, maybe using something like clang-tidy-diff.py in combination with something like the .rst linter to reduce the iterations on a revision, speed up development and reduce the burden  that seems present on a group of dedicated but possible select people.

That would be nice, I mean the issue with Phab is that Arcanist is difficult/not-nice to use (IMO), and I think most of its uses of conduit APIs here are pretty limited, would be nice to have something like a lighter Arcanist in python, that doesn't require a local PHP installation. Without changing Phab itself it would be a nice place to add things like pre-diff submission hooks, ideally with something people can extend that fits into their build systems/CI. Would be an interesting project as far as review efficiency goes. At least in my opinion, I tried using Arcanist for like two commits and went back to manual after since it just felt clunky. As far as code style goes, a lot of parts like MCStreamer are difficult to automatically format and in some places it can produce unfavorable results, so I'm 50/50 on automatically running a linter on diffs since the actual diff may come out weird. Readability is a big aspect to consider and I've seen a lot of code produced by clang-format that isn't necessarily easy to read, but is committed anyway because clang-format is considered to be a de-facto tool for ensuring code style is followed. A lot of corner cases, for example in MC layer in LLVM, an automatic linter may be less desirable than someone familiar with the style guide manually formatting the patch. Again just an opinion, not sure how others feel about that.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D56482