[PATCH] D46699: [ThinLTO] Print module summary index to assembly

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/IR/AsmWriter.cpp:2776
+static std::string getLinkagePrintName(GlobalValue::LinkageTypes LT,
+                                       bool AddSpace = true) {
+  std::string Space = AddSpace ? " " : "";
----------------
tejohnson wrote:
> dexonsmith wrote:
> > pcc wrote:
> > > tejohnson wrote:
> > > > pcc wrote:
> > > > > Maybe changing the other callers to do:
> > > > > ```if (GV->getLinkage() != GlobalValue::ExternalLinkage)
> > > > >    Out << getLinkagePrintName(GV->getLinkage()) << ' ';
> > > > > ```
> > > > > would be clearer than adding the argument?
> > > > I thought about that but felt it was less churn and clearer to handle it in one centralized place. If you much prefer changing the callers, I can do that instead.
> > > Can you do that please? I do prefer to avoid mysterious bool arguments wherever possible.
> > I agree with Peter that the `bool` argument is mysterious.  We also don't tend to pass around `std::string`s much.
> > 
> > Here's a third option that should be less boilerplate.  First, in a prep NFC patch, do:
> > - Simplify this to `const char *getLinkageName(GlobalValue::LinkageTypes)`.
> > - Add a function, `printLinkageWithSpace(raw_ostream&,GlobalValue::LinkageTypes)`.  Its implementation can call `getLinkageName` and add a space, but return early for `ExternalLinkage`.
> > - Update current users to call `printLinkageWithSpace`.
> > 
> > Then in your patch:
> > - Change `getLinkageName` to return `"external"` for `ExternalLinkage`.
> > - Use `getLinkageName` where you need it.
> I like this suggestion - Peter, does that work for you?
Yes, that works for me, thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D46699