Closed SirNickolas closed 9 months ago
Attention: 3 lines
in your changes are missing coverage. Please review.
Comparison is base (
498790f
) 98.42% compared to head (64339ef
) 98.54%. Report is 5 commits behind head on 2.x.
Files | Patch % | Lines |
---|---|---|
source/argparse/internal/argumentuda.d | 90.90% | 3 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for this effort. I've never had time to look at it. Please be aware that there is not yet committed change where I rewrote parser.d so it will affect this PR. I'm rebasing changes and will push them as soon as I'm done.
Everything is fine; it’s not ready yet to look at it anyway. Rewriting parser.d
will affect this PR minimally: it focuses primarily on command.d
and argumentuda.d
. Perhaps valueparser.d
as well, but not sure yet.
FYI I'm planning to merge 2.x branch into master as I'm done with major changes and IMO it makes sense to have everything in default branch.
Iʼll keep that in mind, thanks.
For the record, in the last few days Iʼve been optimizing pure templates (those that do not produce any code in the binary) – rewriting TypeTraits
, to be exact. However, this saved pitiful 50 MB so I discarded those changes. (I still have (the essential part of) the source though.)
Trying a different approach now. It is showing prominent results.
Thanks to your recent refactoring, even unpatched 2.x
requires only 2.7 GB RAM to build. The first commit in this PR reduces it to 2.3 GB. And then a few surgical changes…
I think I’ll stop here.
For the best end-user experience, I’d suggest moving tests from argparse/package.d
to a separate module that is not in sourcePaths
and therefore not distributed with the rest of the library. That would reduce compilation time and memory consumption for clients (0.8 GB → 0.4 GB, which is what I would expect from a CLI library) as well as resolve #125.
To sum up:
HelpArgumentUDA.parse
: it doesn’t need to statically know its config, and it doesn’t even look at the receiver.callParser
.)COMMAND_STACK
is always Command[]
everywhere).CLI.complete
.)That was an amusing journey.
P.S. I also thought about splitting Config
into two structs to reduce amount of fields that must be known at compile time. For example, at the moment everything is compiled twice: for StylingMode.on
and StylingMode.off
. But I was already satisfied with the results and haven’t done this.
By the way, now I understand that DMD was correct here. CLI!(config, C).complete
instantiates CLI!(config, Complete!C)
. CLI!(config, Complete!C).complete
in turn instantiates CLI!(config, Complete!(Complete!C))
, and so on… You just ran out of memory faster than DMD figured out the recursion is infinite.
Now that complete
is a template (not semchecked until necessary), this issue is gone.
For the best end-user experience, I’d suggest moving tests from
argparse/package.d
to a separate module that is not insourcePaths
and therefore not distributed with the rest of the library.
I think the better approach would be to move those unit tests to other modules. I've been adding new unit tests there but didn't pay attention to remove them from package.d
. I'll take a look at it.
I also thought about splitting
Config
into two structs to reduce amount of fields that must be known at compile time.
I looked at this some time ago and it didn't look promising at that point because I found working on parser rewriting will give greater benefit so I started working on that. May it makes sense to review that idea again since I'm done with rewriting.
If I make both branches identical (passing true
to enableStyling
), it compiles 15% faster and produces a 15% slimmer binary. I expect splitting Config
will have approximately this impact.
Upd.: Measured for a unittest
build, which instantiates hundreds of commands. For a regular case of just 1 command, the difference is negligible. I still believe that compilation time matters for unittest
too.
If I make both branches identical (passing
true
toenableStyling
), it compiles 15% faster and produces a 15% slimmer binary. I expect splittingConfig
will have approximately this impact.Upd.: Measured for a
unittest
build, which instantiates hundreds of commands. For a regular case of just 1 command, the difference is negligible. I still believe that compilation time matters forunittest
too.
I think we can live with that for now. I'd rather look at possibility of splitting Config
- I think it should be possible to extract a part that affects parsing settings and the rest can be used as runtime parameters.
Just curious, is added argument soup (
scope
nothrow
pure
@safe
etc) required?
A library should correctly list attributes for its public APIs, or otherwise it would be disruptive to users who wish to use those attributes. There are few things more frustrating in D programming than getting a compiler error third_party.someFunc is not safe, peeking at the implementation, and finding out there were no reasons not to declare it safe. Yet it is @system
, and it poisons the whole user’s call stack with systemness. While @safe
, scope
, and const
are quite easy to fake in userland via a @trusted
wrapper (even a lambda suffices), lack of nothrow
, pure
, or @nogc
is trickier to deal with.
That being said, I think it is tolerable to leave out attributes for UDA-related API since it is supposed to be called at struct-declaration time, when it doesn’t matter. I’m also ready to drop them from private/package functions where it doesn’t affect API if you are not comfortable with them: while I think it is always good to have more semantic checks from the compiler, that is my own opinion, and I admit I’ve pushed it too far here.
I think we can live with that for now. I'd rather look at possibility of splitting
Config
I do not propose enabling colors always/never. That was just a quick test to see how much we can gain from splitting the config.
I think we can live with that for now. I'd rather look at possibility of splitting
Config
I do not propose enabling colors always/never. That was just a quick test to see how much we can gain from splitting the config.
No worries, I got your point :)
Just curious, is added argument soup (
scope
nothrow
pure
@safe
etc) required?A library should correctly list attributes for its public APIs, or otherwise it would be disruptive to users who wish to use those attributes. There are few things more frustrating in D programming than getting a compiler error third_party.someFunc is not safe, peeking at the implementation, and finding out there were no reasons not to declare it safe. Yet it is
@system
, and it poisons the whole user’s call stack with systemness. While@safe
,scope
, andconst
are quite easy to fake in userland via a@trusted
wrapper (even a lambda suffices), lack ofnothrow
,pure
, or@nogc
is trickier to deal with.
That being said, I think it is tolerable to leave out attributes for UDA-related API since it is supposed to be called at struct-declaration time, when it doesn’t matter. I’m also ready to drop them from private/package functions where it doesn’t affect API if you are not comfortable with them: while I think it is always good to have more semantic checks from the compiler, that is my own opinion, and I admit I’ve pushed it too far here.
I don't mind having these attributes. Could you please do the following:
No problem. I excluded that commit from this PR.
Is it too late to ask you to split this PR into separate smaller/simpler PRs that do single thing each? It's hard to review big one since you did a lot of things.
I hope it became more manageable now.
I checked improvements on my WSL Ubuntu: memory usage during dub test
dropped from 4 GB to 1 GB. Impressive!
In general I prefer to leave
Config
as template parameter in parser unless moving it to runtime parameters gives feasible benefits. One item in my TODO list is to try splittingConfig
into smaller parts with "single responsibility", for example: there is definitely one part that affects parser that IMO should stay as template parameter and there is also a "styling" part that can absolutely be runtime parameter.
Hm, I see your point. To be honest, I haven’t looked at the parser’s source carefully yet; was planning to do it after we decide on #117. I’ll return Config
to parser’s template parameters for now.
Smaller parts with single responsibility sounds great. For example, namedArgPrefix
should be statically known by TypeTraits
(so that they can validate that argument names do not begin with it) while it can be a runtime parameter for the parser (it doesn’t get any benefits from statically knowing it).
By the way, I wonder whether this code is correct. If ValueParser == void
, shouldn’t we return ArgumentUDA!T(newInfo)
? Apparently, no test stresses this.
By the way, I wonder whether this code is correct. If
ValueParser == void
, shouldn’t we returnArgumentUDA!T(newInfo)
? Apparently, no test stresses this.
I think ArgumentUDA!void
is used in unit tests only. There is no reason to have this in real code and I believe there is no way to even have that.
Just a FYI: I plan to merge 2.x
branch to master
after this PR so all following work (if any) should go to default branch.
Memory usage dropped from ∞ to 2.9 GB DMD + 0.25 GB linker, but I want to reduce it to a sane limit. Stay tuned.
Closes #122 when it is complete.