<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>First (these comments apply more to the -xverbose patch, but
we've been discussing these issues here):<br>
<br>
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.<br>
<br>
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.<br>
<br>
And...<br>
</p>
<div class="moz-cite-prefix">On 7/17/22 21:19, Kris Van Hees wrote:<br>
</div>
<blockquote type="cite" cite="mid:YtTfSXMDdm70Xsux@oracle.com">
<pre class="moz-quote-pre" wrap="">On Sun, Jul 17, 2022 at 05:43:41PM -0700, Eugene Loh wrote:
</pre>
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">On 7/16/22 21:39, Kris Van Hees wrote:
</pre>
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">On Fri, Jul 15, 2022 at 07:37:17PM -0700, Eugene Loh via DTrace-devel wrote:
</pre>
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">For this one:
Reviewed-by: Eugene Loh <a class="moz-txt-link-rfc2396E" href="mailto:eugene.loh@oracle.com" moz-do-not-send="true"><eugene.loh@oracle.com></a>
since it's good enough as is.
</pre>
</blockquote>
</blockquote>
</blockquote>
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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 <b class="moz-txt-star"><span class="moz-txt-tag">*</span>not<span class="moz-txt-tag">*</span></b> want -xcpp to take effect.</pre>
</blockquote>
<br>
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?<br>
<br>
<blockquote type="cite" cite="mid:YtTfSXMDdm70Xsux@oracle.com">
<pre class="moz-quote-pre" wrap="">I can add a comment in the implementation plan to indicate that -xverbose is
not modified by -xdisasm=N.</pre>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite" cite="mid:YtTfSXMDdm70Xsux@oracle.com">
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">*) When the cpp fix is introduced, it should be in final form,
without having to be amended in a subsequent patch.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
<br>
I guess I do not see the value of:<br>
<br>
*) Using the same solution for both problems.<br>
<br>
*) 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."<br>
<br>
Anyhow, thanks for the discussion.<br>
</body>
</html>