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

[Mandeep Singh Grang via Phabricator via llvm-commits]

mgrang 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:
> 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.


================
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;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D26214