dmd icon indicating copy to clipboard operation
dmd copied to clipboard

Avoid unnecessary template

Open ryuukk opened this issue 1 year ago • 6 comments

ryuukk avatar Oct 10 '24 04:10 ryuukk

Thanks for your pull request and interest in making D better, @ryuukk! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#16982"

dlang-bot avatar Oct 10 '24 04:10 dlang-bot

src/dmd/root/string.d(29): Error: no property `fTuple` for `str` of type `const(char)[]`

RazvanN7 avatar Oct 10 '24 05:10 RazvanN7

Perhaps the right approach is to update the format function in chkformat.d so it understands D's strings?

You can't pass D slices to C-style variadic arguments. Even if you change that, the compiler is still being compiled with dmd version 2.079, so dmd's source can't rely on new features.

The motivation for the helper function is:

  • Reduce argument list length (notice how this PR bumps lines past 120 columns)
  • Extract side effects (toString().length, toString().ptr calls toString twice)
  • Ease future refactoring (when e.g. transitioning from C-style to D-style variadics, it becomes easy to adapt arguments)

What's the motivation for removing it in this PR?

dkorpel avatar Oct 10 '24 08:10 dkorpel

Reading library.filename.fTuple.expand means nothing, yet hides ton of complexity, argument count will be 2 instead of 1

I don't think a length, ptr pair is a ton of complexity. Perhaps fTuple is a bit cryptic on first read, but printfStringTuple is a bit long for something that is supposed to make string passing more convenient. I'm open to better names.

you mention issues that are non existent in your PR

There's refactoring PRs converting C strings to D strings all the time. The idea is that it can be used for future PRs as well, not just the one PR that introduced it.

The only way to prevent the future issues you mention is to make the function understand what a D string is, not to tell people to instead rely on this cryptic code

I want to, believe me, but for the time being we're stuck with printf and C variadics.

I don't think this is worth discussing at length, perhaps @RazvanN7 and @thewilsonator can weigh in on whether it is too obfuscating, and if it is, we can merge this and I'll stop refactoring printf calls like this.

dkorpel avatar Oct 10 '24 10:10 dkorpel

The PR stalled, and as I already explained, I don't think this is worth spending much time on, so I didn't want to ask you to rebase, update, and continue discussing this.

Dictator dkorpel wants to enforce his template magic to... print a string?

I'm just trying to get work done. From my perspective, you're the one constantly interjecting and complaining that other people's Pull Requests don't follow your personal coding principles, without understanding the circumstances/constraints of the DMD source (C++ interface, bootstrap compiler etc.).

We expect professional demeanor in DLF's forums / GitHub repositories by the way. I warned you before: if you continue to post messages that are cynical or contain personal attacks, I will start deleting / blocking.

dkorpel avatar Feb 18 '25 14:02 dkorpel