[PATCH] D49116: Setup clang-format as an Arcanist linter

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: .arcanist/linters/clang-format.sh:12-13
+  NC='\033[0m' # No Color
+  echo -e "${RED}Please install clang-format-diff for arcanist to lint C/C++ source code.${NC}" >&2
+  exit 1
+fi
----------------
starsid wrote:
> dberris wrote:
> > What happens when the script exits with a non-zero value? Does it stop the upload/update of the patch? I suspect we don't really want to make that a hard requirement yet, unless we get agreement from llvm-dev that this is something we should enforce for all patches moving forward.
> When the script exits with a non-zero value (which should only happen if clang-format-diff is not installed), the user's upload workflow is stopped, and the user is messaged saying that the linter failed.
> 
> I have now added a message saying that the user can skip linting by passing `--nolint` to `arc diff`. So it's not a hard requirement, but it is a little disruptive. I can make it silently fail by returning an exit code of 0 instead of 1, but I don't feel like it's a good idea.
I would push back on this a bit -- let's make it noisy when you don't have clang-format installed, but not stop the upload flow and provide a way to silence the warning. When we (llvm-dev) decide that we will require clang-format'ing all new patches through arcanist/Phabricator then we can change the default.


================
Comment at: .arclint:8
+      "include": [
+        "(\\.(cpp|h)$)"
+      ]
----------------
dberris wrote:
> Should this also include '.cc' files?
I don't see the change to support .cc files...


Repository:
  rL LLVM

https://reviews.llvm.org/D49116