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

[Eric Christopher via Phabricator via llvm-commits]

echristo 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('=');
----------------
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.


================
Comment at: llvm/trunk/lib/MC/MCContext.cpp:266
+    if (Pair.second.empty()) {
+      errs() << "error: defsym must be of the form: sym=value: " << I << "\n";
+      return 1;
----------------
mgrang wrote:
> grosbach wrote:
> > The MC layer should not be writing directly to errs(), but rather going through the diagnostics handlers.
> @grosbach I will change this to go through diagnostics handlers.
> 
> However, I just moved this function from llvm-mc.cpp as is (almost). llvm-mc was already writing to errs(). Also I see places in MC where we directly write to errs(), like in MC/SubtargetFeature.cpp.
llvm-mc is an application and not a library. MC itself should not be writing to errs().

The uses in SubtargetFeature should be fixed, not promulgated further.


Repository:
  rL LLVM

https://reviews.llvm.org/D26214