[PATCH] D125421: [TableGen] Add generation of argument register lists

[Bill Wendling via Phabricator via llvm-commits]

void marked 3 inline comments as done.
void added inline comments.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:71-75
+      CurrentAction = CC->getName().str();
+      // Call upon the creation of a map entry from the void!
+      // We want an entry in AssignedRegsMap for every action, even if that
+      // entry is empty.
+      AssignedRegsMap[CurrentAction] = {};
----------------
nickdesaulniers wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > Can we sink these two statements
> > > ```
> > > CurrentAction = CC->getName().str();
> > > AssignedRegsMap[CurrentAction] = {};
> > > ```
> > > into the definition of `EmitCallingConv`?
> > Okay, but could we keep the review to issues about the algorithm and not coding style please?
> Whatever.  Code style is an inseparable part of code review.  Otherwise it makes my review approval just a rubber stamp. And if it's just a rubber stamp, why even waste my time asking for review?
But you ask for changes that make no real difference in the code. It's like you're doing it because you think you MUST find something to comment on, otherwise it'll be a "rubber stamp", which isn't true. I'm not a junior level programmer. When I do things it's for a reason. I put these two statements where they were because the information is global, so it made more sense to me to place them in a global setting and not sink them down into a called function. You may disagree with that, but that's just what it is, a disagreement.

What I need from a review is someone to look at the code and say, "Oh! this could be better done this way," or "You forgot to account for this." I'm not really looking for someone to say, "This line of code could use this format and not this format," when the result is the same. If there's an egregious violation of the style guide, then sure point that out. But otherwise, it's okay to allow people to program in their own fashion.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:106-108
+                                 std::string Name =
+                                     Class.first->getNameInitAsString();
+                                 return StringRef(Name).startswith("CCIfSwift");
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > void wrote:
> > > nickdesaulniers wrote:
> > > > ```
> > > > StringRef Name = Class.first->getNameInitAsString();
> > > > return Name.startswith("CCIfSwift");
> > > > ```
> > > > Not sure if that would fit on one line if it was a single statement?
> > > It goes over 80 columns.
> > I still think the pair of expressions need not make any mention of `std::string` if you just construct a `StringRef` from the get go.
> s/expressions/statements/
Oh, sorry. I misunderstood what you wanted. Done.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:332-353
+restart:
+  for (EntryTy &Entry : DelegateToMap) {
+    StringRef RegName = Entry.first();
+    std::set<std::string> &Registers = Entry.second;
+    if (!Registers.empty())
+      continue;
+
----------------
nickdesaulniers wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > i.e. I _think_ you could avoid the goto here with:
> > > > 
> > > > ```
> > > > for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); Entry != E; ++E) {
> > > >   StringRef Regname = It->first();
> > > >   ...
> > > >   DelegateToMap.erase(RegName.str());
> > > > }
> > > > ```
> > > > if DelegateToMap was a std::map.
> > > > https://stackoverflow.com/a/54004916
> > > > (Iterator invalidation being the weak point to range-for)
> > > > for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); Entry != E; ++E) {
> > > 
> > > Sorry, only renamed one of the expressions, should have been
> > > ```
> > > for (auto *It = DelgateToMap.begin(), E = DelegateToMap.end(); It != E; ++It) {
> > > ```
> > See comment about `goto` above.
> I still think this could be rewritten to avoid the use of `goto`.
You were right. Sorry about that. Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421