Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Commit

Permalink
Use fixed size array of parsers instead of dynamic stack array
Browse files Browse the repository at this point in the history
  • Loading branch information
@philsquared
philsquared committed Oct 21, 2017
1 parent ac294c2 commit 7d5daa1
Showing 1 changed file with 11 additions and 5 deletions.
16 changes: 11 additions & 5 deletions include/clara.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,18 +822,24 @@ namespace detail {
size_t count = 0;
};
const size_t totalParsers = m_options.size() + m_args.size();
ParserInfo parseInfos[totalParsers];
size_t i = 0;
for( auto const& opt : m_options ) parseInfos[i++].parser = &opt;
for( auto const& arg : m_args ) parseInfos[i++].parser = &arg;
assert( totalParsers < 512 );
// ParserInfo parseInfos[totalParsers]; // <-- this is what we really want to do
ParserInfo parseInfos[512];

{
size_t i = 0;
for (auto const &opt : m_options) parseInfos[i++].parser = &opt;
for (auto const &arg : m_args) parseInfos[i++].parser = &arg;
}

m_exeName.set( exeName );

auto result = InternalParseResult::ok( ParseState( ParseResultType::NoMatch, tokens ) );
while( result.value().remainingTokens() ) {
bool tokenParsed = false;

for( auto& parseInfo : parseInfos ) {
for( size_t i = 0; i < totalParsers; ++i ) {
auto& parseInfo = parseInfos[i];
if( parseInfo.parser->cardinality() == 0 || parseInfo.count < parseInfo.parser->cardinality() ) {
result = parseInfo.parser->parse(exeName, result.value().remainingTokens());
if (!result)
Expand Down

3 comments on commit 7d5daa1

@trueqbit
Copy link

@trueqbit trueqbit commented on 7d5daa1 Oct 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::array would already give you debug checks, so the assertion could be omitted.

What is speaking against BMBurstein's solution to use a vector (see PR #27 )? Yes, it does incur a runtime allocation cost, but given these are pointers it's kind of negligible? It has the advantage that you don't have to check for the maximum number of parsers (which should be done in 'release' mode as well).

@philsquared
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course! std::array does exactly what I want here (forgot about that in my rush to back out the fixed dynamic array). Thanks, @trueqbit.

As for why not to use a vector - that's what I was originally doing (albeit in a slightly different version of the code) but in the first round of reviews a surprising number of people (well, >1) expressed that it would be desirable if the whole library could be used without heap allocations. This had not occurred to me before as I thought they would not be a bottleneck in the context of a command line parser - but the request was about certain embedded platforms where they are problematic.

It's not there yet - there are a few other places that will lead to allocations - but this was just part of my first sweep of removing unnecessary allocations.

@trueqbit
Copy link

@trueqbit trueqbit commented on 7d5daa1 Oct 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request and your argument make sense :) I am no expert on embedded programming, how is std::string treated on those platforms?

However there seems to exist a generic solution for VLAs, which also VC++ supports: have you considered alloca?

Please sign in to comment.