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

[Bill Wendling via Phabricator via llvm-commits]

void added inline comments.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:29
+  std::map<std::string, std::set<std::string>> AssignedSwiftRegsMap;
+  std::map<std::string, std::set<std::string>> DelegateToMap;
+
----------------
nickdesaulniers wrote:
> I know the current code is using `std::string` without explicitly including `<string>`, but would you mind please cleaning that up here (in this patch)?
> 
> Also curious if we should be preferring containers from ADT?
I *really* hate adding headers that aren't necessary. Is this some kind of style guide violation?


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:73
+      CurrentAction = CC->getName().str();
+      (void)AssignedRegsMap[CurrentAction];
       EmitCallingConv(CC, O);
----------------
nickdesaulniers wrote:
> What is this code doing? Trying to create a key without an subsequent entry? Suspicious.
Yes. I want it to have an entry, even if empty, for all actions that we're processing. I'll make the comment clearer.


================
Comment at: llvm/utils/TableGen/CallingConvEmitter.cpp:327
+void CallingConvEmitter::EmitArgRegisterLists(raw_ostream &O) {
+  using EntryTy = std::pair<std::string, std::set<std::string>>;
+
----------------
nickdesaulniers wrote:
> This is a common case for just using `auto &` in your range for loops.
I wish people would make up their minds about whether I should use `auto` or not. One day I shouldn't use it then the next I should. I don't see why I would suddenly use it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125421