Improve compilation time and memory #124

SirNickolas · 2023-11-19T13:04:06Z

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.

codecov · 2023-11-19T13:06:13Z

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              2.x     #124      +/-   ##
==========================================
+ Coverage   98.42%   98.54%   +0.11%     
==========================================
  Files          25       25              
  Lines        1718     1790      +72     
==========================================
+ Hits         1691     1764      +73     
+ Misses         27       26       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrey-zherikov · 2023-11-20T12:18:42Z

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.

SirNickolas · 2023-11-20T12:28:09Z

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.

andrey-zherikov · 2023-11-22T04:42:15Z

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.

SirNickolas · 2023-11-22T12:35:16Z

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.

SirNickolas · 2023-11-22T20:08:42Z

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:

Consider if a function really needs its template parameters or if it can work with runtime parameters just fine. The function body is copied for each unique combination of template parameters so we should strive to keep them minimal. An extreme example is HelpArgumentUDA.parse: it doesn’t need to statically know its config, and it doesn’t even look at the receiver.
If a template parameter is unavoidable, try to keep the function body short. It is sometimes possible to forward to another function that has fewer template parameters. (Example: callParser.)
Template parameters that are always bound to the same value don’t harm much (e.g., COMMAND_STACK is always Command[] everywhere).
If a function is executed at compile time, it’s better to do it via CTFE (regular parameters) than via template expansion (template parameters) since the compiler doesn’t need to cache CTFE results. There will also be difference in speed when the CTFE engine in D becomes smarter. (I hope it will happen at some point, thus saying when, not if…)
In contrast, we can turn something into a template to avoid type checking and code generation if that thing is never used. (Example: 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.

SirNickolas · 2023-11-23T07:59:34Z

argparse/source/argparse/api/cli.d

Lines 155 to 160 in 498790f

    
           int complete(string[] args) 
        
           { 
        
               import std.sumtype: match; 
        
               // dmd fails with core.exception.OutOfMemoryError@core\lifetime.d(137): Memory allocation failed 
        
               // if we call anything from CLI!(config, Complete!COMMAND) so we have to directly call parser here

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.

andrey-zherikov · 2023-11-25T20:04:06Z

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.

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.

SirNickolas · 2023-11-26T12:58:17Z

argparse/source/argparse/internal/parser.d

Lines 620 to 623 in eb44f11

    
           if(detectSupport()) 
        
               return callParser!(enableStyling(config, true), completionMode, COMMAND)(receiver, args, unrecognizedArgs); 
        
           else 
        
               return callParser!(enableStyling(config, false), completionMode, COMMAND)(receiver, args, unrecognizedArgs);

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.

andrey-zherikov

Just curious, is added argument soup (scope nothrow pure @safe etc) required?

source/argparse/internal/style.d

source/argparse/internal/command.d

andrey-zherikov · 2023-11-26T18:24:44Z

argparse/source/argparse/internal/parser.d

Lines 620 to 623 in eb44f11

if(detectSupport())

return callParser!(enableStyling(config, true), completionMode, COMMAND)(receiver, args, unrecognizedArgs);

else

return callParser!(enableStyling(config, false), completionMode, COMMAND)(receiver, args, unrecognizedArgs);

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.

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.

SirNickolas · 2023-11-27T07:00:38Z

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.

SirNickolas · 2023-11-27T08:53:18Z

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.

See andrey-zherikov#124 for more information.

andrey-zherikov · 2023-11-28T03:06:09Z

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 :)

andrey-zherikov · 2023-11-28T03:14:58Z

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 don't mind having these attributes. Could you please do the following:

Move adding attributes to separate PR. This will allow having more targeted PRs.
Since this allows some specific/additional use cases (e.g. in safe code), it should be checked and proved with unit tests so please add (or update existing) unit tests to validate the use case.

source/argparse/api/argumentgroup.d

source/argparse/api/command.d

source/argparse/api/restriction.d

See andrey-zherikov#124 for more information.

SirNickolas · 2023-11-28T10:00:31Z

No problem. I excluded that commit from this PR.

andrey-zherikov · 2023-11-28T18:39:22Z

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.

This helps in reducing both compilation time and (build-time) memory requirement.

2.3 GB -> 1.7 GB.

1.7 GB -> 1.6 GB.

1.6 GB -> 0.8 GB.

SirNickolas · 2023-11-28T20:22:31Z

I hope it became more manageable now.

andrey-zherikov · 2023-11-29T03:18:51Z

I checked improvements on my WSL Ubuntu: memory usage during dub test dropped from 4 GB to 1 GB. Impressive!

andrey-zherikov

I'm done with the first commit only. Will continue reviewing as time permits.

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 splitting Config 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.

andrey-zherikov · 2023-11-28T21:15:21Z

source/argparse/internal/arguments.d

    {
-        static assert(info.shortNames.length + info.longNames.length > 0);
+        assert(info.shortNames.length + info.longNames.length > 0);


Note for possible future changes (not required in this PR): this should be moved to TypeTraits

source/argparse/internal/valueparser.d

andrey-zherikov · 2023-11-28T21:38:41Z

source/argparse/internal/restriction.d


-    private void initialize(Config config, ArgumentInfo[] infos)()
+    private void initialize(ref const Config config, const(ArgumentInfo)[] infos)


Do we need ref here for config?

It will work either way. I have no strong reasons to either keep ref or drop it. Put it here just because Config is a 168-byte struct.

andrey-zherikov · 2023-11-28T21:44:29Z

source/argparse/internal/commandinfo.d


-    static assert(name == "" || getCommandInfo.names.length > 0 && getCommandInfo.names[0].length > 0, "Command "~COMMAND.stringof~" must have name");
+    assert(name == "" || info.names.length > 0 && info.names[0].length > 0, "Command "~COMMAND.stringof~" must have name");


Note for future possible changes: move this to TypeTraits

source/argparse/internal/argumentuda.d

source/argparse/internal/parser.d

andrey-zherikov

Other commits were much more manageable :-D
They look good.

source/argparse/internal/completer.d

SirNickolas · 2023-12-01T15:20:23Z

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 splitting Config 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).

So that the compiler can prune dead branches of code.

source/argparse/internal/parser.d

source/argparse/internal/argumentuda.d

Renamed into `finalize` in the process. It only fills `ArgumentInfo` so it doesn't have to know about UDAs at all.

SirNickolas · 2023-12-08T10:54:54Z

source/argparse/internal/argumentuda.d

+        alias uda0 = defaultUDA;
+        enum checkMinMax0 = true; // Passed `defaultUDA` always has undefined `minValuesCount`/`maxValuesCount`


defaultUDA == NamedArgument() always so we can drop this parameter and assign uda0 = ArgumentUDA!(ValueParser!(void, void, void, void, void, void)).init instead.

defaultUDA == NamedArgument() was done to avoid dependency cycle between .d modules. Maybe it's not needed anymore but I didn't check.

Got it. I think it can be solved by splitting argumentuda.d into two files (struct ArgumentUDA(T) vs getMemberArgumentUDA). Will do it after we are done with this PR.

SirNickolas · 2023-12-08T12:39:32Z

argparse/source/argparse/internal/argumentuda.d

Lines 30 to 33 in 7cff352

    
           static if(is(ValueParser == void)) 
        
               return ArgumentUDA!ValueParser(newInfo); 
        
           else 
        
               return ArgumentUDA!(ValueParser.addDefaults!T)(newInfo);

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.

andrey-zherikov · 2023-12-08T17:39:08Z

argparse/source/argparse/internal/argumentuda.d

Lines 30 to 33 in 7cff352

static if(is(ValueParser == void))

return ArgumentUDA!ValueParser(newInfo);

else

return ArgumentUDA!(ValueParser.addDefaults!T)(newInfo);

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.

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.

source/argparse/internal/argumentuda.d

source/argparse/internal/command.d

andrey-zherikov · 2023-12-09T18:10:44Z

source/argparse/internal/argumentuda.d

    static assert(udas.length <= 1, "Member "~TYPE.stringof~"."~symbol~" has multiple '*Argument' UDAs");
+    static assert(typeUDAs.length <= 1, "Type "~MemberType.stringof~" has multiple '*Argument' UDAs");

    static if(udas.length > 0)


Maybe convert this if this way since defaultUDA always has undefined minValuesCount/maxValuesCount?

static if(udas.length > 0) enum uda0 = udas[0]; else alias uda0 = defaultUDA; enum checkMinMax0 = uda0.info.minValuesCount.isNull || uda0.info.maxValuesCount.isNull;

It’s on my checklist but can’t be done right now: defaultUDA is not compile-time-known, we cannot assign a value derived from it to an enum.

np, we can do this later

source/argparse/internal/argumentuda.d

andrey-zherikov · 2023-12-10T14:34:15Z

source/argparse/internal/argumentuda.d

-                uda.info.placeholder = symbol.toUpper;
-            }
-        }
+    static if(udas.length > 0)


As an idea:

Suggested change

static if(udas.length > 0)

static if(udas.length > 0)

alias memberUDA = () => udas[0];

else

alias memberUDA = () => defaultUDA;

static if(typeUDAs.length > 0)

alias addTypeUDA = _ => _.addDefaults(typeUDAs[0]);

else

alias addTypeUDA = _ => _;

alias finalize = _ { _.info = _.info.finalize!MemberType(config, symbol); return _; };

enum uda = memberUDA.addTypeUDA.finalize;

static if(uda.info.minValuesCount.isNull || uda.info.maxValuesCount.isNull)

{

alias addMinMax = _ {

if(_.info.minValuesCount.isNull) _.info.minValuesCount = defaultValuesCount!MemberType.min;

if(_.info.maxValuesCount.isNull) _.info.maxValuesCount = defaultValuesCount!MemberType.max;

return _;

};

return uda.addMinMax;

}

else

return uda;

This looks nice, but unfortunately, you’ll get an error on line enum uda = memberUDA.addTypeUDA.finalize since defaultUDA is not available at compile time.

N.B. UFCS does not work for local symbols; should be finalize(addTypeUDA(memberUDA)) instead.

Yeah, it's a drawback. I think it would be better to go that way although I don't mind it to be improved in follow up PR.

andrey-zherikov · 2023-12-10T14:35:50Z

source/argparse/internal/argumentuda.d


-            uda.info.displayNames = chain(uda.info.shortNames, uda.info.longNames).map!toDisplayName.array;
-        }
+    static if(checkMinMax0 && checkMinMax1)


The whole point of the code under this if is to set min/max if they are not set. Please see my comment above - I hope it's clearer and avoids using checkMinMax* by just having simple static if (min/max is not set).

I’d suggest to leave it as is for now. When I get rid of defaultUDA, this code will become prettier as well.

No objections

source/argparse/internal/argumentuda.d

andrey-zherikov · 2023-12-10T17:43:38Z

source/argparse/internal/arguments.d

+        alias toDisplayName = _ => ( _.length == 1 ? config.namedArgPrefix ~ _ : text(config.namedArgPrefix, config.namedArgPrefix, _));
+
+        info.displayNames = chain(info.shortNames, info.longNames).map!toDisplayName.array;


Found bug in code unrelated to your changes:

Suggested change

alias toDisplayName = _ => ( _.length == 1 ? config.namedArgPrefix ~ _ : text(config.namedArgPrefix, config.namedArgPrefix, _));

info.displayNames = chain(info.shortNames, info.longNames).map!toDisplayName.array;

info.displayNames = info.shortNames.map!(_ => config.namedArgPrefix ~ _) ~

info.longNames.map!(_ => text(config.namedArgPrefix, config.namedArgPrefix, _));

I agree it is better now, but could you explain why it is a bug please? Just curious.

I split names into shortNames and longNames in order to implement supporting of multi-character short names. This feature is not implemented yet so this bug was to be discovered. I'll take care of it later - my comment was mostly for me.

OK. Apparently, ArgumentInfo will need a constructor taking string[] names.

Also, in finalize, chain(info.shortNames.map!(...), info.longNames.map!(...)).array will result in a single allocation while ...array ~ ...array will need three.

andrey-zherikov · 2023-12-10T18:03:20Z

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.

* Refactor (#96) * Move onError tp api/cli.d * Improve ANSI styling handling and making AnsiStylingArgument boolean-like * Make Parser private * Remove hooks * Remove subcommands * Remove argumentparser * Update readme * Add unit tests * Rename Config.helpStyle to Config.styling (#97) * Add unit test (#98) * Refactor (#99) * Small cleanup * Make Config a template parameter * Add unit test * Styling in error messages (#100) * Add errorMessagePrefix to Style * Rename Style.namedArgumentName => Style.argumentName * Styling in error messages * Add check for argument name (#101) * Rename Config.namedArgChar to namedArgPrefix (#102) * Add checks for positional arguments (#103) * Refactor (#105) * Add ArgumentInfo.memberSymbol * Small refactoring * Move Restriction and RestrictionGroup to internal.restriction * Remove symbol parameter * remove partial apply * Small refactoring * Small refactoring * Add unit test * Pin LDC to 1.34.0 (#108) * Add '--verbose' to builds * Refactoring (#127) * Required positional arguments must come before optional ones * Optional positional arguments are not allowed with default subcommand * Rewrite parser (#128) * Refactor * Split ArgumentInfo.names to shortNames and longNames * Add namedArguments and positionalArguments to Arguments * Rename Arguments.arguments to info * Refactor Arguments API * Remove ArgumentInfo.ignoreInDefaultCommand * Rewrite parser * unit tests * Update readme (#121) * Update readme * Update the examples as well * Apply suggested changes to readme * Declare `Style.Default` with an alternative syntax (#130) * Turn `main` and `mainComplete` into regular templates (#132) They don't need advanced features that template mixins provide. (Regular templates are mixable, too.) https://dlang.org/spec/template-mixin.html * Make Styling API `nothrow pure @safe` (#133) * Reduce allocations in Styling API (#134) * Reduce allocations in Styling API * Remove the overload of `TextStyle.opCall` that takes a sink We should make `StyledText` a range instead. * Do not depend on `std.regex` (#131) * Do not depend on `std.regex` This saves 1.5 MB in the binary, which is desirable since not every program that uses `argparse` may want to use regexes - or that particular implementation of regexes. `argparse.ansi.getUnstyledText` became eager, but the library wasn't exploiting its laziness anyway. * Make `getUnstyledText` lazy and `@nogc` * Use constants instead of hardcoded characters * Rework the auxiliary function Co-Authored-By: Andrey Zherikov <andrey-zherikov@users.noreply.github.com> * Remove one mutable variable * Add a small comment --------- Co-authored-by: Andrey Zherikov <andrey-zherikov@users.noreply.github.com> Co-authored-by: Andrey Zherikov <andrey.zherikov@gmail.com> * Improve compilation time and memory (#124) * Use regular parameters instead of template parameters where possible This helps in reducing both compilation time and (build-time) memory requirement. * Deduplicate `HelpArgumentUDA.parse` 2.3 GB -> 1.7 GB. * Deduplicate `Complete.CompleteCmd` 1.7 GB -> 1.6 GB. * Compile the completer on demand 1.6 GB -> 0.8 GB. * Simplify `CLI!(...).complete` * Deduplicate `CounterParsingFunction` * Make some of the config's fields statically known by the parser So that the compiler can prune dead branches of code. * Remove `assignChar` from parser's template parameters * Try to simplify min-max-handling logic in `getMemberArgumentUDA` * Add a unit test for `getMemberArgumentUDA` * Move `getArgumentUDA` to `argparse.internal.arguments` Renamed into `finalize` in the process. It only fills `ArgumentInfo` so it doesn't have to know about UDAs at all. * Import non-std modules once per file --------- Co-authored-by: Nickolay Bukreyev <SirNickolas@users.noreply.github.com> Co-authored-by: Andrey Zherikov <andrey-zherikov@users.noreply.github.com>

* Use regular parameters instead of template parameters where possible This helps in reducing both compilation time and (build-time) memory requirement. * Deduplicate `HelpArgumentUDA.parse` 2.3 GB -> 1.7 GB. * Deduplicate `Complete.CompleteCmd` 1.7 GB -> 1.6 GB. * Compile the completer on demand 1.6 GB -> 0.8 GB. * Simplify `CLI!(...).complete` * Deduplicate `CounterParsingFunction` * Make some of the config's fields statically known by the parser So that the compiler can prune dead branches of code. * Remove `assignChar` from parser's template parameters * Try to simplify min-max-handling logic in `getMemberArgumentUDA` * Add a unit test for `getMemberArgumentUDA` * Move `getArgumentUDA` to `argparse.internal.arguments` Renamed into `finalize` in the process. It only fills `ArgumentInfo` so it doesn't have to know about UDAs at all. * Import non-std modules once per file

SirNickolas force-pushed the comptime branch from 09c6805 to 913f6e7 Compare November 20, 2023 11:56

SirNickolas force-pushed the comptime branch from 913f6e7 to d17eeaf Compare November 22, 2023 20:05

SirNickolas marked this pull request as ready for review November 22, 2023 20:14

andrey-zherikov reviewed Nov 26, 2023

View reviewed changes

source/argparse/internal/style.d Outdated Show resolved Hide resolved

source/argparse/internal/command.d Show resolved Hide resolved

source/argparse/internal/command.d Outdated Show resolved Hide resolved

SirNickolas added a commit to SirNickolas/argparse.d that referenced this pull request Nov 27, 2023

Drop attributes from internal functions and UDA API

4ed3e86

See andrey-zherikov#124 for more information.

andrey-zherikov reviewed Nov 28, 2023

View reviewed changes

source/argparse/api/argumentgroup.d Outdated Show resolved Hide resolved

source/argparse/api/argumentgroup.d Outdated Show resolved Hide resolved

source/argparse/api/command.d Outdated Show resolved Hide resolved

source/argparse/api/restriction.d Outdated Show resolved Hide resolved

SirNickolas force-pushed the comptime branch from 81e987f to 1e6c259 Compare November 28, 2023 09:59

SirNickolas added a commit to SirNickolas/argparse.d that referenced this pull request Nov 28, 2023

Drop attributes from internal functions and UDA API

cf312b0

See andrey-zherikov#124 for more information.

SirNickolas added 6 commits November 29, 2023 03:14

Use regular parameters instead of template parameters where possible

3e24b86

This helps in reducing both compilation time and (build-time) memory requirement.

Deduplicate HelpArgumentUDA.parse

a1f4870

2.3 GB -> 1.7 GB.

Deduplicate Complete.CompleteCmd

045f0a1

1.7 GB -> 1.6 GB.

Compile the completer on demand

e1768ed

1.6 GB -> 0.8 GB.

Simplify CLI!(...).complete

0d2a28d

Deduplicate CounterParsingFunction

879a161

SirNickolas force-pushed the comptime branch from 1e6c259 to 879a161 Compare November 28, 2023 20:22

andrey-zherikov reviewed Nov 30, 2023

View reviewed changes

source/argparse/internal/completer.d Show resolved Hide resolved

source/argparse/internal/completer.d Show resolved Hide resolved

Make some of the config's fields statically known by the parser

73fe0ce

So that the compiler can prune dead branches of code.

andrey-zherikov reviewed Dec 4, 2023

View reviewed changes

source/argparse/internal/parser.d Outdated Show resolved Hide resolved

source/argparse/internal/argumentuda.d Outdated Show resolved Hide resolved

source/argparse/internal/argumentuda.d Outdated Show resolved Hide resolved

SirNickolas added 4 commits December 8, 2023 16:44

Remove assignChar from parser's template parameters

ce6a171

Try to simplify min-max-handling logic in getMemberArgumentUDA

e73805e

Add a unit test for getMemberArgumentUDA

f5ac263

Move getArgumentUDA to argparse.internal.arguments

53ccf7e

Renamed into `finalize` in the process. It only fills `ArgumentInfo` so it doesn't have to know about UDAs at all.

SirNickolas commented Dec 8, 2023

View reviewed changes

andrey-zherikov reviewed Dec 10, 2023

View reviewed changes

Import non-std modules once per file

64339ef

andrey-zherikov approved these changes Dec 11, 2023

View reviewed changes

andrey-zherikov merged commit 9bc9ccb into andrey-zherikov:2.x Dec 11, 2023
8 checks passed

SirNickolas deleted the comptime branch December 12, 2023 13:30


		private void initialize(Config config, ArgumentInfo[] infos)()
		private void initialize(ref const Config config, const(ArgumentInfo)[] infos)


		static assert(name == "" \|\| getCommandInfo.names.length > 0 && getCommandInfo.names[0].length > 0, "Command "~COMMAND.stringof~" must have name");
		assert(name == "" \|\| info.names.length > 0 && info.names[0].length > 0, "Command "~COMMAND.stringof~" must have name");

		alias uda0 = defaultUDA;
		enum checkMinMax0 = true; // Passed `defaultUDA` always has undefined `minValuesCount`/`maxValuesCount`

-    static if(udas.length > 0)
+    static if(udas.length > 0)
+        alias memberUDA = () => udas[0];
+    else
+        alias memberUDA = () => defaultUDA;
+    static if(typeUDAs.length > 0)
+        alias addTypeUDA = _ => _.addDefaults(typeUDAs[0]);
+    else
+        alias addTypeUDA = _ => _;
+    alias finalize = _ { _.info = _.info.finalize!MemberType(config, symbol); return _; };
+    enum uda = memberUDA.addTypeUDA.finalize;
+    static if(uda.info.minValuesCount.isNull || uda.info.maxValuesCount.isNull)
+    {
+        alias addMinMax = _ {
+            if(_.info.minValuesCount.isNull) _.info.minValuesCount = defaultValuesCount!MemberType.min;
+            if(_.info.maxValuesCount.isNull) _.info.maxValuesCount = defaultValuesCount!MemberType.max;
+            return _;
+        };
+        return uda.addMinMax;
+    }
+    else
+        return uda;

		alias toDisplayName = _ => ( _.length == 1 ? config.namedArgPrefix ~ _ : text(config.namedArgPrefix, config.namedArgPrefix, _));

		info.displayNames = chain(info.shortNames, info.longNames).map!toDisplayName.array;

Improve compilation time and memory #124

Improve compilation time and memory #124

Conversation

SirNickolas commented Nov 19, 2023

codecov bot commented Nov 19, 2023 • edited Loading

Codecov Report

andrey-zherikov commented Nov 20, 2023

SirNickolas commented Nov 20, 2023 • edited Loading

andrey-zherikov commented Nov 22, 2023

SirNickolas commented Nov 22, 2023

SirNickolas commented Nov 22, 2023 • edited Loading

SirNickolas commented Nov 23, 2023

andrey-zherikov commented Nov 25, 2023

SirNickolas commented Nov 26, 2023 • edited Loading

andrey-zherikov left a comment

Choose a reason for hiding this comment

andrey-zherikov commented Nov 26, 2023

SirNickolas commented Nov 27, 2023 • edited Loading

SirNickolas commented Nov 27, 2023

andrey-zherikov commented Nov 28, 2023

andrey-zherikov commented Nov 28, 2023

SirNickolas commented Nov 28, 2023

andrey-zherikov commented Nov 28, 2023

SirNickolas commented Nov 28, 2023

andrey-zherikov commented Nov 29, 2023

andrey-zherikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrey-zherikov left a comment

Choose a reason for hiding this comment

SirNickolas commented Dec 1, 2023

SirNickolas Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirNickolas commented Dec 8, 2023

andrey-zherikov commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirNickolas Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrey-zherikov commented Dec 10, 2023

codecov bot commented Nov 19, 2023 •

edited

Loading

SirNickolas commented Nov 20, 2023 •

edited

Loading

SirNickolas commented Nov 22, 2023 •

edited

Loading

SirNickolas commented Nov 26, 2023 •

edited

Loading

SirNickolas commented Nov 27, 2023 •

edited

Loading

SirNickolas Dec 8, 2023 •

edited

Loading

SirNickolas Dec 11, 2023 •

edited

Loading