ldrumm added a comment.

In http://reviews.llvm.org/D15527#324919, @spyffe wrote:

> I'm a little concerned that LanguageRuntime::GetOverrideExprOptions() appears 
> to generate a new set of options from whole cloth rather than using the 
> existing set as a starting point.
>
> Specifically, since your use case is wanting to override the calling 
> convention, I think it's heavy handed to override all the options.  I could 
> conceive of new expression parser features changing some other flags in the 
> future, and it'd be unfortunate if we had to duplicate those features in the 
> ActionScript code just because the options are being overridden.


That's definitely a good point!

> Could we model this as OverrideExprOptions(clang::TargetOptions &) instead 
> and modify the options in place?  That way customizations could stack.


I think this is a really helpful suggestion, but I'm slightly foggy on the 
semantic changes you propose.
If we go ahead, and implement `OverrideExprOptions(clang::TargetOptions &)` 
what type do you think the virtual method  should return as a default value? I 
like the semantics of the `nullptr` meaning "nothing going on here", and I 
think it's conceptually easier to specialize instances of OverrideExprOptions 
in individual runtimes with the default base implementation return `nullptr` in 
case the user doesn't know or care (though this may be only a personal 
preference. AFAICS this will also make it easier to pass around pointers to the 
base class, while having specialized  instances available to be constructed in 
the `Runtime dynamically`.

I propose that we incorporate your suggestion and pass in a ``const 
clang::TargetOptions &`` which allows the runtime to learn about its 
environment, but allows dynamic allocation in subclasses, where this is 
appropriate. Perhaps also incorporating use of ``std::shared_ptr`` to track 
ownership of the returned object would be helpful here to improve the 
management style.

Thanks

Luke


http://reviews.llvm.org/D15527



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to