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

[Eric Christopher via llvm-commits]

On Thu, Dec 1, 2016 at 8:46 PM Tim Northover via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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.
>
>
Agreed.


>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D26214
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161202/41ba9dec/attachment.html>