[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