Minor improvement [Reserve vector]

[shahzadlone]

Save on reallocation cost, by reserving the vector we know the size of before hand.

[jhasse]

Thanks for the PR! While you're right, that this improves performance, the code path isn't hot and performance isn't really an issue there. Also calling Python takes the majority of the time anyway.

[jhasse]

performance isn't really an issue there.

What difference does it make if python is more expensive?

It results in the impact of reserve becoming unmeasurable.

[shahzadlone]

@jhasse I agree this might not be a hot path, that being said this is better code than before, even if it might be on a small magnitude. This is either good (probably is) or does no negative effect at all.

I assumed the end goal usually is to achieve for better code. Even if the change is as small as changing a post-inc (i++) to pre-inc(++i), one can make an argument that compilers can optimize that but still you shouldn't compromise on code quality.

Anyways, even if this isn't merge I can understand. But could you please update the contribution guide lines to say you guys don't accept minor improvements.

[jhasse]

It hard-codes the number of arguments, so it's not necessarely "better" code.

[shahzadlone]

Number of arguments, are already hard coded.

If you would like to guide me to a better approach, I would be happy to fix this up.

[shahzadlone]

Thanks, sure i will do that @jonesmz

Your fork has ++14!? I thought this repository supported ++11, also I have only used string_view's for C++17 projects, does ++14 support it aswell?

[jhasse]

Number of arguments, are already hard coded.

I meant the number 7.

[jhasse]

The number of arguments is hard-coded at 7 + argc. Why does it matter?

When adding another parameter, one has to remember changing 7 to 8.

[jhasse]

No actually it wouldn't. The only consequence would be an additional allocation.

Exactly ;)

There are times when performance is critical and times where developing comfort is more important. For the latter people often choose Python. This shows that this part of Ninja, is not meant to be performance-critical. So "better" code in this case is the one that is simpler, shorter, prettier, etc. IMHO using reserve in this case is overkill and unneeded.

I think if you would benchmark the change, there wouldn't be a measurable performance impact, making this a case of premature optimization.

[jhasse]

So again, you should have said all of that to the person who wrote the pull request initially.

??? This isn't a DM or private chat, I'm replying on the PR.

And it still doesn't mean we should not improve this.

Sure, but I think that "performance" should be weighted very low for deciding what is an improvement to this code.

Is the suggestion that I made, such that the number of arguments is automatically handled if they change, acceptable or not? You can see a version of that suggestion here: jonesmz#1

His version without the reserve would be even shorter and simpler, so I would prefer that one. Both won't work with C++98, so we can't merge them for now, sorry.

[jhasse]

What about this won't work with C++98?

Nothing.

btw: If you add cpp after the first three backticks, the code snippet gets highlighted on GitHub.

[shahzadlone]

shahzadlone@7ffd251

This won't be updated here because this is closed, should I make a new PR ?

[shahzadlone]

Perfect then, because this commit was on the same branch this pull request was made on.

[shahzadlone]

@jhasse Please guide me with further instructions.

[jhasse]

@shahzadlone The version I prefer requires C++11, so we'll have to wait until Ninja drops C++98 support.

[jhasse]

It's longer and harder to understand IMHO.

Are there plans for C++11, btw?

No concrete ones. Support for Debian 8 ends on June 30, 2020, that might be a date to keep in mind.

[shahzadlone]

@jhasse It is better code though, and what is so hard to understand about that code? It also fixes the problem of hard coding things which you yourself pointed out "When adding another parameter, one has to remember changing 7 to 8."

[jhasse]

It is better code though

That's subjective. I think we can agree that good code should be

1. short,
2. easy to understand,
3. and performant.

This is only better on 3. The weighting of those three factors is subjective, I gave some reasoning why I weight performance very low here.

what is so hard to understand about that code?

I didn't say it's hard to understand. It's harder.