[DTrace-devel] [PATCH 04/12] Fix the -xcpp option

[Eugene Loh]

First (these comments apply more to the -xverbose patch, but we've been 
discussing these issues here):

1)  It seems we should NOT say that -xverbose and -s are equivalent in 
light of their different behaviors and your explanations below how they 
are so different.

2)  Even if we want something to take place at multiple sites, we can 
maintain that "something" in one place and then call it from however 
many places we want.

And...

On 7/17/22 21:19, Kris Van Hees wrote:
> On Sun, Jul 17, 2022 at 05:43:41PM -0700, Eugene Loh wrote:
>> On 7/16/22 21:39, Kris Van Hees wrote:
>>
>>> On Fri, Jul 15, 2022 at 07:37:17PM -0700, Eugene Loh via DTrace-devel wrote:
>>>> For this one:
>>>>       Reviewed-by: Eugene Loh<eugene.loh at oracle.com>
>>>> since it's good enough as is.
>> One is that it does not make sense for the same code to be replicated in
>> multiple places -- in this case, in both
>>      dtrace_program_fcompile()
>>      dtrace_program_strcompile()
>> After all, they're both setting up for a call to dt_compile(), which has
>> access to both cflags and dtp.  So, the work can be done there, as far as
>> cpp is concerned.  Or in a different function altogether that can be called
>> from the two functions.  In any case, we should not be maintaining the same
>> code in two places.
> But you overlook the fact that dt_compile() is also called from the
> dtrace_type_strcompile() and dtrace_type_fcompile() functions, and in those
> cases you do*not*  want -xcpp to take effect.

I'm puzzled by this objection.  Putting the check in dt_compile() is 
precisely what this patch does, isn't it?  Are you objecting to this patch?

> I can add a comment in the implementation plan to indicate that -xverbose is
> not modified by -xdisasm=N.

I guess that's okay, but it would be better if explanations were part of 
more persistent artifacts -- at least commit messages and code but 
ideally user docs.  And, again, it sounds like we simply should not say 
that -S and -xverbose are equivalent.

>> *)  When the cpp fix is introduced, it should be in final form,
>>      without having to be amended in a subsequent patch.
> I deliberately implemented this patch series to show the development.  With
> just -xcpp to consider, the more generic approach introduced in the -xverbose
> patch was not necessary.  Only when -xverbose was implemented did it become
> clear that the same approach could be used for both.  The patch series reflects
> that.

I guess I do not see the value of:

*)  Using the same solution for both problems.

*)  Using a solution in one patch that is backed out in (basically) the 
next patch.  Progressive development is nice, but in this case it's "add 
code, now remove it."

Anyhow, thanks for the discussion.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.oracle.com/pipermail/dtrace-devel/attachments/20220718/2987d359/attachment.html>