https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112020-04-21T18:00:25ZRuby Issue Tracking SystemRuby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=852362020-04-21T18:00:25ZEregon (Benoit Daloze)
<ul></ul><p>I think as much as feasible macros accessing internals should not be in public headers.<br>
OTOH, I think macros purely for portability between platforms, compilers, C/C++ compatibility are fine.</p>
<p><code>RUBY3_STATIC_ASSERT</code> seems harmless (just a help for a messy C/C++ language and portability) but anything poking at the internals of, e.g., RString is harmful.<br>
Harmful because it prevents further optimizations,<br>
it increases maintenance cost as it become time-consuming to remove/change,<br>
it increases the cost for alternative implementations to support it (e.g., TruffleRuby),<br>
and it increase the chance for 3rd-party extensions misusing it (= more bugs).</p>
<p>As we've seen with the so-called "intern.h" header it's now considered public API, so 3rd-party extensions seem to take for granted that anything in public headers is public API.</p>
<p>Could you give some pointers or example of other macros introduced in that change?<br>
Are they mostly about portability, or also about accessing internals?</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=852592020-04-23T08:30:10Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Eregon (Benoit Daloze) wrote in <a href="#note-1">#note-1</a>:</p>
<blockquote>
<p>Could you give some pointers or example of other macros introduced in that change?<br>
Are they mostly about portability, or also about accessing internals?</p>
</blockquote>
<p>Almost every new macros are for portability. For instance <code>RUBY3_HAS_BUILTIN</code> is another macro that got publicised this time. Its former name was <code>__has_builtin</code>, which of course mimics clang behaviour for everybody else. There are also newly introduced inline functions. They are refactored bits of former macros, hence they touch internals of e.g. RString.</p>
<p>If you want the complete list, new macros are:</p>
<pre><code>git grep '^# *define RUBY3.*[^H]$'
</code></pre>
<p>and new inline functions are:</p>
<pre><code>git grep '^ruby3_.*)$'
</code></pre>
<p>Also, I left this paragraph to every new header files introduced:</p>
<blockquote>
<p>Symbols prefixed with either <code>RUBY3</code> or <code>ruby3</code> are<br>
implementation details. Don't take them as canon. They could<br>
rapidly appear then vanish. The name (path) of this header file<br>
is also an implementation detail. Do not expect it to persist<br>
at the place it is now. Developers are free to move it anywhere<br>
anytime at will.</p>
</blockquote>
<p>I believe I made it clear that 3rd parties shall never use those names. No enforcement though.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853592020-05-02T12:06:55ZEregon (Benoit Daloze)
<ul></ul><p>shyouhei (Shyouhei Urabe) wrote in <a href="#note-2">#note-2</a>:</p>
<blockquote>
<p>If you want the complete list, new macros are:</p>
<pre><code>git grep '^# *define RUBY3.*[^H]$'
</code></pre>
</blockquote>
<p>I took a look, these look fine to me, they seem almost all about portability, except <code>RUBY3_ANYARGS_DISPATCH*</code>.</p>
<p>I think most of those could even be public portability helper macros (with a different prefix then, like <code>RB_</code>), and they wouldn't be hard to support long term as MRI needs them anyway.</p>
<blockquote>
<p>and new inline functions are:</p>
<pre><code>git grep '^ruby3_.*)$'
</code></pre>
</blockquote>
<p>Few of these, but the naming seems like they are more likely to be used by native extensions.<br>
For example, intuitively <code>ruby3_str_new_cstr()</code> sounds like a "the Ruby 3 way/the new C-API to create a Ruby String from a C String".<br>
But it's not, it's just an internal function and implementation detail.</p>
<p>I think the only issue here is that <code>ruby3_/RUBY3_</code> is fairly unclear, unless people read that comment.<br>
But I can easily imagine they might miss that comment due to looking at the middle of the file, or just ignore it.</p>
<p>I think we should consider other prefixes which might be clearer.<br>
<code>ruby3_/RUBY3_</code> sounds too much like "the new way to do things/the new C-API" when it's not.</p>
<p>Maybe <code>ruby_internal_/RUBY_INTERNAL_</code>?<br>
That's quite long, but those macros/functions are already long, and maybe a longer name is intuitively clearer it shouldn't be used in native extensions (plus, you know, there is "internal" in it).</p>
<p>Any thought on using <code>ruby_internal_/RUBY_INTERNAL_</code>?</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853702020-05-03T08:26:32Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Thank you for looking at the new macros/functions.</p>
<p>Eregon (Benoit Daloze) wrote in <a href="#note-3">#note-3</a>:</p>
<blockquote>
<p>I took a look, these look fine to me, they seem almost all about portability, except <code>RUBY3_ANYARGS_DISPATCH*</code>.</p>
</blockquote>
<p>Yes. ANYARGS related ones were explicitly converted to have RUBY3_* prefix, to make them something non-standard. I don't think we want people to use them directly.</p>
<blockquote>
<p>I think most of those could even be public portability helper macros (with a different prefix then, like <code>RB_</code>), and they wouldn't be hard to support long term as MRI needs them anyway.</p>
</blockquote>
<p>Or maybe they could be separated into different header-only library, like ccan. I guess they are ruby-agnostic.</p>
<blockquote>
<p>For example, intuitively <code>ruby3_str_new_cstr()</code> sounds like a "the Ruby 3 way/the new C-API to create a Ruby String from a C String".<br>
But it's not, it's just an internal function and implementation detail.</p>
<p>I think the only issue here is that <code>ruby3_/RUBY3_</code> is fairly unclear, unless people read that comment.<br>
But I can easily imagine they might miss that comment due to looking at the middle of the file, or just ignore it.</p>
<p>I think we should consider other prefixes which might be clearer.<br>
<code>ruby3_/RUBY3_</code> sounds too much like "the new way to do things/the new C-API" when it's not.</p>
<p>Maybe <code>ruby_internal_/RUBY_INTERNAL_</code>?<br>
That's quite long, but those macros/functions are already long, and maybe a longer name is intuitively clearer it shouldn't be used in native extensions (plus, you know, there is "internal" in it).</p>
<p>Any thought on using <code>ruby_internal_/RUBY_INTERNAL_</code>?</p>
</blockquote>
<p>I chose <code>RUBY3</code>/<code>ruby3</code> only to avoid jamming my fingers by typing some long names. I feel for instance <code>ruby_internal_right_shift_is_arithmetic_p()</code> is a bit too long. So while I don't stick at <code>3</code>, I would prefer something shorter than <code>_internal</code>.</p>
<p>Looking at other projects, Boost for instance uses <code>detail</code> for this purpose. It seems there are other projects using <code>impl</code>. What about those names, like <code>ruby_impl</code> or even, <code>rbimpl</code>?</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853712020-05-03T14:48:15ZEregon (Benoit Daloze)
<ul></ul><p>shyouhei (Shyouhei Urabe) wrote in <a href="#note-4">#note-4</a>:</p>
<blockquote>
<p>What about those names, like <code>ruby_impl</code> or even, <code>rbimpl</code>?</p>
</blockquote>
<p>I think those are already much clearer than <code>ruby3_</code>.<br>
<code>ruby_internal_</code> might be even clearer but indeed it's long.</p>
<p>I think <code>rbimpl_</code> is a good compromise.<br>
Would it be OK to change the prefix to that?<br>
I think it's a great improvement over the <code>ruby3_</code> prefix.</p>
<p>To help comparing visually:</p>
<pre><code class="c syntaxhl" data-language="c"><span class="k">static</span> <span class="kr">inline</span> <span class="kt">long</span>
<span class="nf">rb_fix2long</span><span class="p">(</span><span class="n">VALUE</span> <span class="n">x</span><span class="p">)</span>
<span class="p">{</span>
<span class="k">if</span> <span class="p">(</span><span class="n">ruby3_right_shift_is_arithmetic_p</span><span class="p">())</span> <span class="p">{</span>
<span class="k">return</span> <span class="n">ruby3_fix2long_by_shift</span><span class="p">(</span><span class="n">x</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">if</span> <span class="p">(</span><span class="n">ruby_impl_right_shift_is_arithmetic_p</span><span class="p">())</span> <span class="p">{</span>
<span class="k">return</span> <span class="n">ruby_impl_fix2long_by_shift</span><span class="p">(</span><span class="n">x</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">if</span> <span class="p">(</span><span class="n">rbimpl_right_shift_is_arithmetic_p</span><span class="p">())</span> <span class="p">{</span>
<span class="k">return</span> <span class="n">rbimpl_fix2long_by_shift</span><span class="p">(</span><span class="n">x</span><span class="p">);</span>
<span class="p">}</span>
<span class="k">if</span> <span class="p">(</span><span class="n">ruby_internal_right_shift_is_arithmetic_p</span><span class="p">())</span> <span class="p">{</span>
<span class="k">return</span> <span class="n">ruby_internal_fix2long_by_shift</span><span class="p">(</span><span class="n">x</span><span class="p">);</span>
<span class="p">}</span>
</code></pre> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853752020-05-04T08:46:06Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p><a href="https://github.com/ruby/ruby/pull/3079" class="external">https://github.com/ruby/ruby/pull/3079</a></p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853782020-05-05T05:32:11Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><p>If it was me, I'd literally call it <code>ruby/internal/file.h</code> to make it very clear, along with <code>ruby3</code> -> <code>rb_internal_/RB_INTERNAL_</code></p>
<p>Assuming <code>impl</code> means implementation, it's not clear that it's private, but <code>internal</code> does mean a certain kind of private.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853792020-05-05T06:43:11Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>ioquatix (Samuel Williams) wrote in <a href="#note-7">#note-7</a>:</p>
<blockquote>
<p>If it was me, I'd literally call it <code>ruby/internal/file.h</code> to make it very clear, along with <code>ruby3</code> -> <code>rb_internal_/RB_INTERNAL_</code></p>
<p>Assuming <code>impl</code> means implementation, it's not clear that it's private, but <code>internal</code> does mean a certain kind of private.</p>
</blockquote>
<p>That arrangement could be confusing when <a class="user active user-mention" href="https://bugs.ruby-lang.org/users/4">@nobu (Nobuyoshi Nakada)</a> merges his <a href="https://github.com/nobu/ruby/tree/feature/src-dir" class="external">https://github.com/nobu/ruby/tree/feature/src-dir</a> branch. You should persuade him first.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853852020-05-05T21:39:22ZEregon (Benoit Daloze)
<ul></ul><p>shyouhei (Shyouhei Urabe) wrote in <a href="#note-6">#note-6</a>:</p>
<blockquote>
<p><a href="https://github.com/ruby/ruby/pull/3079" class="external">https://github.com/ruby/ruby/pull/3079</a></p>
</blockquote>
<p>Looks a lot clearer to me :)</p>
<p>I think indeed having <code>ruby/internal/attr/const.h</code> instead of <code>ruby/impl/attr/const.h</code> would be even clearer,<br>
and for the directory name I expect the length to type doesn't matter much.</p>
<p>Regarding @nobu's branch, I see it moves <code>internal/vm.h</code> → <code>include/internal/vm.h</code> and so we'd have <code>include/internal/</code> and <code>include/ruby/internal/</code>.<br>
Maybe slightly confusing but I also think it makes sense: both are internal, and one of them is <code>make install</code>-ed and the other not, because <code>include/ruby/internal/</code> needs to be public headers to compile correctly.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=853862020-05-05T22:56:22Zioquatix (Samuel Williams)samuel@oriontransfer.net
<ul></ul><blockquote>
<p>also think it makes sense: both are internal, and one of them of make install-ed and the other not, because include/ruby/internal/ needs to be public headers to compile correctly.</p>
</blockquote>
<p>Agreed. I think because <a class="user active user-mention" href="https://bugs.ruby-lang.org/users/4">@nobu (Nobuyoshi Nakada)</a> was going to use <code>internal</code> we should also use <code>internal</code> here to be consistent.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=854542020-05-08T07:56:32Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>OK then, let's move the directory from <code>impl</code> to <code>internal</code>. Will update the pull request.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=855402020-05-12T20:41:39Zko1 (Koichi Sasada)
<ul></ul><p>Please respect current convention <code>rb_</code>/<code>RB_</code> are called in ruby interpreter (such as <code>rb_str_new()</code>) and <code>ruby_</code>/<code>RUBY_</code> can be called from outside of ruby interpreter (such as <code>ruby_xmalloc()</code>). <code>RUBY3_...</code> are already renamed, but I want to note it here.</p>
<p>PS. I like <code>rb_internal_</code> because it is clearer. <code>rb_internal_</code> functions are not exposed, so I don't think the length is important.</p> Ruby master - Misc #16803: Discussion: those internal macros reside in public API headershttps://bugs.ruby-lang.org/issues/16803?journal_id=856212020-05-14T12:27:09ZEregon (Benoit Daloze)
<ul></ul><p>ko1 (Koichi Sasada) wrote in <a href="#note-12">#note-12</a>:</p>
<blockquote>
<p>Please respect current convention <code>rb_</code>/<code>RB_</code> are called in ruby interpreter (such as <code>rb_str_new()</code>) and <code>ruby_</code>/<code>RUBY_</code> can be called from outside of ruby interpreter (such as <code>ruby_xmalloc()</code>). <code>RUBY3_...</code> are already renamed, but I want to note it here.</p>
</blockquote>
<p>I think very few people know this, is this documented somewhere? It would be nice to document it e.g., in <code>ruby.h</code> or <code>ruby/ruby.h</code>.</p>
<blockquote>
<p>PS. I like <code>rb_internal_</code> because it is clearer. <code>rb_internal_</code> functions are not exposed, so I don't think the length is important.</p>
</blockquote>
<p><code>rb_internal_</code> looks nice, but it also begins with <code>rb_</code> which gives a "public API" feeling. So <code>rbimpl_</code> seems better visually as "don't use (externally)".</p>