[PATCH] D121758: [clang-format] Add support for formatting Verilog code

Fri Mar 18 02:22:24 PDT 2022

MyDeveloperDay added a comment.

In D121758#3389181 <https://reviews.llvm.org/D121758#3389181>, @sstwcw wrote:

> Yes.  I am surprised that you asked since everyone asked me to break it apart.

Well I was thinking you move the unrelated parts out and then reduce the side of this patch, (but you'll have patch dependencies but then the review will be more manageable for us.)

No one is saying that adding Verilog isn't a good idea. (it is actually as long as its not too invasive), I like @HazardyKnusperkeks  suggestion of bringing it in in smaller pieces, I could even see us landing pieces before
it was fully complete. (Like we did with C#, which probably isn't 100% complete either, but we incrementally add it)

If people don't try and format Verilog (which they shouldn't be expecting to then it won't hurt if its not 100% complete.) And from my perspective is seems it NOT possible for you to land Verilog without completely rewriting large
swaths of the seemingly unrelated other code first? is the structure of clang-format so bad that you can't possible work with what is there without having to come in a refactor it all from underneath us?

Bottom line, here is the impression  I get... You likely have a downstream fork of clang-format that supports Verilog, and you are trying to upstream it..problem is as you developed it you refactored a lot of things (to some extent for the better in your view), But you didn't add unit tests for those refactoring because frankly you didn't need to it was your local copy.

Now landing those refactoring seems like a good idea to you, but we have to take them on sight unseen that those refactoring are ok, With no unit tests to back up your justification even if that just means bolstering the existing tests with new tests that cover more cases. (your isIf() change is one, I'd be HIGHLY surprised if that didn't cause some sort of regressions for someone somewhere, when I say regression I mean changes, both positive and negative but people call them regression! either way even if we fix stuff..)

I feel like we are trying to be super responsive to your reviews (no seriously I used to have to wait weeks for someone to even have a look!, we have a great team of reviewers and contributors they are highly skilled), You've been an LLVM member in Phabricator for not a month. Some people have multiple years experience here, What are we to do? accept everything on face value without unit tests?

But I'm sorry I feel bad, because at every turn I'm like "Really, are we doing that now?  (MACRO is FormatToken)",  I hate doing it but I feel like I'm pushing back on every review. But we can't just let stuff in without trying to do a full and through review and that means unit tests, you understand that right?

Don't expect to drive by with this review, some of my reviews took 1-2 years to land, my dedication to stay and fix bugs in the meantime proved to the reviewers that I was serious. It could take this level of time and commitment to gain peoples trust to land such a large patch.

You are clearly highly capable and understand the clang-format code base. We want you as a contributor because you understand it already. But this is a most unusual onboarding, but I feel I personally we have to treat the reviews as I would any other review coming in...which I'm afraid is with scrutiny.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121758