[PATCH] Add observers to Input Graph
Rui Ueyama
ruiu at google.com
Tue May 13 12:57:27 PDT 2014
================
Comment at: include/lld/Core/InputGraph.h:74
@@ +73,3 @@
+ /// being returned.
+ void registerObserver(std::function<void(File *)> &fn);
+
----------------
Shankar Kalpathi Easwaran wrote:
> Michael Spencer wrote:
> > Simon Atanasyan wrote:
> > > Maybe it is better to use the `llvm::function_ref` class to be consistent with main LLVM code.
> > >
> > > http://llvm.org/docs/ProgrammersManual.html#passing-functions-and-other-callable-objects
> > This is news to me. I would much prefer we use std::function.
> >
> > So after doing some research, it seems that advanced uses of std::function can fail on VC11 (VS 2012). We're not doing that here, and the buildbots have the final say in what is supported.
> You might want to change the observer function to return an ErrorOr,
I'd prefer std::function too.
================
Comment at: include/lld/Core/InputGraph.h:74
@@ +73,3 @@
+ /// being returned.
+ void registerObserver(std::function<void(File *)> &fn);
+
----------------
Rui Ueyama wrote:
> Shankar Kalpathi Easwaran wrote:
> > Michael Spencer wrote:
> > > Simon Atanasyan wrote:
> > > > Maybe it is better to use the `llvm::function_ref` class to be consistent with main LLVM code.
> > > >
> > > > http://llvm.org/docs/ProgrammersManual.html#passing-functions-and-other-callable-objects
> > > This is news to me. I would much prefer we use std::function.
> > >
> > > So after doing some research, it seems that advanced uses of std::function can fail on VC11 (VS 2012). We're not doing that here, and the buildbots have the final say in what is supported.
> > You might want to change the observer function to return an ErrorOr,
> I'd prefer std::function too.
Return type of void is intentional. Observers should never fail, and should not do anything other than observing.
================
Comment at: lib/ReaderWriter/FileArchive.cpp:100-108
@@ -99,1 +99,11 @@
+ /// Returns a set of all defined symbols in the archive.
+ std::set<StringRef> getDefinedSymbols() const override {
+ std::set<StringRef> ret;
+ for (const auto &e : _symbolMemberMap) {
+ StringRef sym = e.first;
+ ret.insert(sym);
+ }
+ return ret;
+ }
+
----------------
Shankar Kalpathi Easwaran wrote:
> This may be put in one line using std::transform.
>
> std::transform(_symbolMemberMap.begin(),_symbolMemberMap.end(), std::back_inserter(ret), [](const auto &pair){return pair.first;});
It looks more cryptic than the plain for loop. I think it's better to keep the for loop. It might make sense to write it in four lines like this.
std::set<StringRef> ret;
for (const auto &e : _symbolMemberMap)
ret.insert(e.first)
return ret;
http://reviews.llvm.org/D3735
More information about the llvm-commits
mailing list