timshen added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:443
+ writeThinLTOBitcode(OS, ThinLinkOS,
+ [&FAM](Function &F) -> AAResults & {
+ return FAM.getResult<AAManager>(F);
----------------
chandlerc wrote:
> Are we unable to deduce the return type here?
No, because return type deduction uses "auto deduction" instead of "decltype
deduction" - it deduces to a value type.
================
Comment at: llvm/tools/opt/NewPMDriver.h:61
+ bool EmitSummaryIndex, bool EmitModuleHash,
+ Optional<tool_output_file *> ThinLTOBC);
}
----------------
chandlerc wrote:
> timshen wrote:
> > chandlerc wrote:
> > > Why an optional pointer? Maybe just allow the pointer to be null if there
> > > isn't a thin link output file?
> > There are 3 states:
> > 1) Use ThinLTOBitcodeWrite pass, controlled by OutputThinLTOBC, and the
> > output file is not null.
> > 2) OutputThinLTOBC is true, but output file is null (don't write to the
> > file).
> > 3) OutputThinLTOBC is false.
> >
> > An optional nullable pointer, confusing as it might be (but I'm not sure
> > how much), provides 3 states.
> >
> > If we instead pass in a bool and a pointer, that's 4 states, one of which
> > is invalid.
> Ok, but is #2 in your list actually useful? Maybe it is, but if it is then at
> the very least the comment should make it super clear what is going on here.
> Otherwise, I suspect many readers will assume than when the optional is
> engaged the pointer isn't null as that's just too "obvious".
#2 is consistent with opt's legacy pass manager behavior (search
"createWriteThinLTOBitcodePass" in opt.cpp).
I added an emphasized "*nullable*" in the comment. :)
https://reviews.llvm.org/D33525
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
