[PATCH] D97657: [lld][WebAssembly] Initial support merging string data

Sat May 1 08:58:40 PDT 2021

dblaikie added inline comments.

================
Comment at: lld/wasm/InputChunks.h:216
+public:
+  SyntheticMergedDataSegment(StringRef name, uint32_t flags_,
+                             uint32_t alignment_)
----------------
sbc100 wrote:
> dblaikie wrote:
> > sbc100 wrote:
> > > dschuff wrote:
> > > > Do we care about these lint checks? (I guess lld already has different style than LLVM?)
> > > We do care yes, but I'm having hard time seeing what is wrong here since we use lowerCamal case for locals and params in lld.   i guess it must be the trailing `_` .. in which case maybe its false positive because it thinks we are using snake_case which would indeed be wrong.   I think we can ignore this one.
> > What's with the underscores though?
> > 
> > I don't think we usually do that in the LLVM codebase - I think we generally name the parameter the same as the variable & ideally use the ctor's initializer list to initialize members. (or use `this->x = x` in a pinch)
> > 
> > Hmm, I guess these members are members of a base class? Perhaps the base class should have a ctor that takes them?
> In this case I'm initialize members of that superclass so I can't use the initializer list (I think).. so I need to disambiguate..   In the past I've used the  underscore suffix disambiguate, but I'm happy to use the `this->x = x` method instead.  I didn't know that was preferred here.
re: base class members: Ah, right, sorry, that's what I was trying to get at when I mentioned "perhaps the base class (aka superclass) should have a ctor that takes them" - if the base class doesn't have a ctor that takes flags and alignment, perhaps you could add one?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97657