[PATCH] D26214: [llvm] Implement support for -defsym assembler option

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: llvm/trunk/lib/MC/MCContext.cpp:263
 
+int MCContext::setSymbolValue(MCStreamer &Streamer, std::string &I) {
+    auto Pair = StringRef(I).split('=');
----------------
grosbach wrote:
> echristo wrote:
> > mgrang wrote:
> > > grosbach wrote:
> > > > The error checking shouldn't be in the setter function, but rather in the caller.
> > > @grosbach Thanks for your comments.
> > > 
> > > This function is called from llvm-mc as well as from cc1as_main in clang. Is it OK if I create an error checker function here and call it from both the call sites? Otherwise we would end up duplicating the error checking logic in both call sites.
> > The applications often have their own error handling. If you want to put something in Support that can be easily handled it's fine, but otherwise it's best if you just duplicate it. Happy to look at code that does either.
> That's not the function of MCContext. The diagnostic checking really should be in the callers even if it's duplicated. Consider, for example, that llvm-mc is a development testing tool for LLVM developers and clang is an end-user tool. It is entirely possible that  we would want different checking and behaviours for those use cases.
> The diagnostic checking really should be in the callers even if it's duplicated

Really, the MC layer shouldn't even be in the business of parsing a user-facing Clang option like this (even if it's guaranteed error-free). Users should provide the symbol and the value separately.


Repository:
  rL LLVM

https://reviews.llvm.org/D26214