[PATCH] D100854: [TableGen] Change assertion information from a tuple to a struct [NFC]

[Paul C. Anagnostopoulos via Phabricator via llvm-commits]

Paul-C-Anagnostopoulos added inline comments.


================
Comment at: llvm/lib/TableGen/TGParser.cpp:368
   if (E.Assertion) {
-    CheckAssert(std::get<0>(*E.Assertion), std::get<1>(*E.Assertion), 
-                std::get<2>(*E.Assertion));
+    CheckAssert((*E.Assertion).AssertionInfo::Loc,
+                (*E.Assertion).AssertionInfo::Condition, 
----------------
dblaikie wrote:
> Paul-C-Anagnostopoulos wrote:
> > dblaikie wrote:
> > > This is some very strange/non-idiomatic code. Why are the member accesses qualified like this?
> > > 
> > > I'd expect this code to look more like: `E.Assertion->Loc` for instance
> > I was hoping someone would tell me how to simplify this. Why do I need 'AssertionInfo::' this way, but not with '->Loc'?
> I don't think you do? You could write it as `(*E.Assertion).Loc` as well. It's pretty uncommon to need to scope a member access like you have here (how'd you came across this syntax/end up writing it this way?) - /sometimes/ it may be useful to scope a member function access (`x.Base::y()`) to access a base member function rather than an override or shadowing in a derived class. (eg: if you want to override a function but delegate some functionality to the orgiinal base function, you could use `Base::func()` to do that delegation - but very rarely for member variable access)
The (*E.Assertion) was just stupid. I did that because *E.Assertion was already there.

For the rest of it. I tried two or three variants until I got it. Let me check . . . nope, don't need the AssertionInfo::. No idea why I ended up with it, since I first assumed I didn't need it.

Twenty years of programming without worrying about pointers has taken its toll.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100854