C++ standard in use?

[jonesmz]

I've been reviewing the source for ninja, and see a lot of areas where c++11 or newer features would be nice to have, but aren't currently used, like the "= default;" keyword for constructors and destructors, ranged based for loops, nullptr, lambdas, constexpr, so on.

What's the maximum c++ version that ninja's codebase abides by?

[evmar]

Ninja mostly just works as-is and doesn't see a lot of code churn, so I don't think it's worth rewriting bits of it in a newer dialect of C++. Maybe Nico feels differently though.

[jonesmz]

Fear of change for the sake of unspecified boogy-bugs doesn't really help much.

I'm considering contributing improvements to ninja, but I'm not going to do that if I'm required to use c++98, and can't take advantage of any language improvements from the last 10 years.

I'm willing to do the work to modernize the codebase, and understand that slow-and-steady wins the race, but I wanted to ask before I jumped in.

[jonesmz]

Considering there are 100 open PRs stretching back to 2013, maybe I shouldn't bother?

[nico]

Ninja is intentionally written in C++98. C++11 is nice, but it means your program can't run on older systems, and build servers for some are often old systems.

Re old PRs: Like all projects, we don't merge all PRs. We keep the ones we don't merge open.

[jonesmz]

When will Ninja adopt C++11?

I'm not asking to be rude, I seriously want to know. I've seen so many projects that use C89 in 2017-2018 for the same justification, but the oldest system the project maintainers know about is CentOS 6.

Are you planning to keep C++98 until 2028?

[jonesmz]

Why are you keeping the PRs you don't merge open?

If the code isn't ever going to be merged, close the PR so people don't see the list and assume "The maintainers of this project ignore PRs", like I just did.

[nico]

There are no plans on adopting C++11. I agree it makes writing code a bit nicer, but ninja's small enough that the benefits of not adopting it (ninja runs on older systems; ninja itself compiles a bit faster) outweigh that.

Re pull requests: Closing a pull request feels unfriendly to me, and there have been instances where I didn't want to merge something at first but changed my mind after enough people explained their use cases over time. Is closing pull request considered best practice? I don't know much about github etiquette.

[jonesmz]

Thank you for the explanation. I disagree with your priorities, but I agree that you've come to the appropriate conclusion based on your stated goals, and I respect that.

As for pull requests, I think it's subjective.

As a "drive by" contributor, my initial reaction on seeing 100 PRs, and the last commit landing 6+ months ago, is that Ninja is a dead project. My understanding of github etiquette is that closing a PR that isn't going to be merged is better than leaving it open.

Were there to have only been 2 or 3 PRs open, and only a few issues open, my assumption (at first glance) would have instead been that the project is mostly stable and there's not much left to do to solve the goals.

Alternatively, if there had been a commit within the last week or so, with a codebase as massive as, for example, the Kodi project, and PRs in the 100s, the impression would have been that the project is under active development, but that there are a lot of proposed changes that aren't measuring up (yet).

For a code base the size of Ninja, I don't think I would be comfortable contributing to a project with more than, e.g. 10 or so open PRs, and certainly none that are open for more than a year. If, out of 10 open PRs, there hasn't been a lot of recent action like a few dozen comments on each, that's also not a great indicator.

In addition to clearly communicating with the person that wrote the PR that the pull request isn't going to be merged in it's current state, it also serves to avoid giving people the impression that the project maintainer doesn't respect contributions.

I'm sure if I went and read through all 100 PRs that I would see some kind of discussion for each of them, but that's, well, time consuming, and I think it's a fair assumption that PRs from 2013 aren't going to merged. PRs from 2017 aren't so easy to tell, but there's quite a lot of those as well.

The 100 PR "backlog" makes people reluctant to contribute to ninja. It gives the impression that nothing gets merged, and that I shouldn't bother.

Note that I'm basing my impressions here on past experience. Projects that have ignored my PRs for months (6 months, 1yr, 2yrs, so on) while carrying on normal development, sometimes accepting other PRs and such. So I'm gun-shy. There's only so much time in a life, I'd rather not spend mine on a project that won't even acknowledge my contributions, and 100 open PRs, some from 2013, makes me very reluctant to participate.

If it weren't for adopting Ninja for my build system at work and want to improve it a bit to integrate better with our existing system, I wouldn't have even started this discussion.

[nafest]

I would appreciate any feedback on a PR. I also didn’t get any comment on #1239, though I use this patch in production for over a year now.

[nico]

Thanks for the thoughtful response. I definitely agree with the "last commit many months ago" part. That's discussed on the list here: https://groups.google.com/forum/#!topic/ninja-build/yUOLRh19M-w (summary: I was away for a while).

https://github.com/ninja-build/ninja/blob/master/HACKING.md#how-to-successfully-make-changes-to-ninja has a blurb that goes """I am very wary of changes that increase the complexity of Ninja (in particular, new build file syntax or command-line flags) or increase the maintenance burden of Ninja. Ninja is already successfully used by hundreds of developers for large projects and it already achieves (most of) the goals I set out for it to do. """.

To restate that a bit, I think it's very important that ninja stays small and simple. The tens (or maybe hundred) of people who submit pull requests sometimes prefer their patch to go in and don't want to hear "what you're adding is a good feature for some build system, but ninja is not that build system". But I think the tens of thousands of people who use ninja appreciate that it mostly Just Works, doesn't break, and sometimes even gets faster over time.

Many pull requests ignore that blurb above (and the "Match the Ninja coding style (see below)") and they add stuff that's useful in some cases, but that also make ninja more complicated. And often what the patch wants to achieve can be achieved without changes to ninja in other ways.

For a while, I left comments on pull requests that said "have you thought about doing X instead; I'd prefer to not merge this patch". This usually resulted in a long-winded discussion which takes up a lot of time. (Sometimes my reluctance is wrong and a feature is actually useful for more people than I thought at first, and a somewhat invasive patch gets merged after enough people say it's useful, as mentioned above.)

So I figured I'd save time by leaving out the long-winded discussion. If a pull request makes an invasive change and doesn't make a strong case why it's worth including in the patch description, I let it sit. I don't know if closing it with a "please read blurb in HACKING.md" would be considered friendlier.

There are of course many patches that I'm very happy about receiving too, and that I usually merge quickly (performance improvements, editor integration improvements, etc.); the above is about patches that add stuff to the manifest, make the internals more complicated without a clear payoff, etc.

[nico]

@nafest I left a comment on your pull request. Since this issue here has morphed a bit into "how should ninja pull requests happen", maybe you can say here if that reply is helpful, what could be different, etc. (I think it's a decent reply, but again it took a while to write, and in a way it's again just a restating of "have you seen HACKING.md".)

[nico]

Oh, and an important addition to my lengthy paragraph above that I wanted to include but then forgot: There are definitely also patches that I want to get in in some form (e.g. Brad King's Fortran work, which I'm merging at a speed that makes "glacial" look fast) but where I'm struggling to make time for reviewing. That's 100% on me.

[nico]

1140 is a current example of a patch that was open for a long time, actually got some testing while sitting around, and where user feedback changed my mind.

[nafest]

@nico thanks, it was very helpful, guess I will try to submit a bug report to Intel. Maybe they can fix my issue.

[jhasse]

I don't know if closing it with a "please read blurb in HACKING.md" would be considered friendlier.

IMHO it's friendlier as nothing is more frustating for me than no comment / reaction at all.

As a general (GitHub) rule I would close PRs that you don't want to merge in the current state, but leave issues open where you think discussion might change your mind, maybe add a "backlog" label to them.

Btw: Maybe you have someone you trust who could help out with going through the issues / PRs?

[nico]

Re others: @evmar can also merge, but he's even busier (and maybe even less committal? :-)) than me. Anyone can go through issues and PRs and leave comments.

[thughes]

Saw the request for feedback on PRs, so here are some thoughts:

For the life of me I couldn't understand why I felt like I was sometimes seeing color output from ninja and other times not. I knew that several years ago I added -fcolor-diagnostics to force color output so ninja would show color output, but it seemed like sometimes it was failing. I finally looked at it a bit today and found that locally I generally run ninja in regular (non verbose) mode and on the build server run in verbose mode (-v) in order to see the full compiler output. Testing locally, it turns out that simply enabling verbose mode disables the color output.

I then decided to poke around the ninja github to see if anyone else had hit this. I ended up spending much longer than I would have liked searching and reading through bugs and pull requests to try to figure out the current state. There was lots of activity, but never any response: https://github.com/ninja-build/ninja/issues/1214 https://github.com/ninja-build/ninja/pull/1268 https://github.com/ninja-build/ninja/pull/1230

From the above listed PRs and bugs, it's impossible for me to tell if "no color output in verbose mode" is a feature or whether a simple change like https://github.com/ninja-build/ninja/pull/1268 would be accepted. Reading through HACKING.md, I see you're hesitant to take new commandline flags, so maybe that's why the changes haven't been taken. Or maybe it's because all the PRs have conflicts and they simply need to be rebased and tested against the latest master. The point is that it's impossible for me to tell. If the PRs had simply been closed with "sorry I'm not going to add more commandline flags" (or whatever reason), then I could saved a lot of time reading. The worst case would be me assuming that the PRs were not merged because they had conflicts and were out-of-date and then spending the time to fix up the existing PRs only to not get any response/feedback.

[jonesmz]

@nico

I don't want to invalidate the discussion that's been had since the topic changed, but can I ask for one more piece of information about the C++ standard used by Ninja?

Can I get a soft commitment on when Ninja will switch to a newer C++ version?

Can I expect C++03 in 2023 or before?

Similarly, C++11 in 2033 or before?

Essentially, I'm looking for a commitment that Ninja will switch to a c++ standard no later than said standard version's 20th birthday. I think a 20-year compatibility period for standard versions is reasonable, no?

[sgraham]

@jonesmz but.. why? Ninja is a tiny code base so it really doesn't matter all that much what dialect or even language it's written in. Maybe try making some substantive changes before fiddling around with style issues?

[jonesmz]

@sgraham

Why does it matter what my motivations behind asking are?

Either the Ninja project will make a soft commitment to adopt a c++ standard version on or before said versions 20th birthday or it won't.

It's a trivial question, with a one word answer. shrug.

As for making substantive changes... Sure, take a look: https://github.com/jonesmz/splinter (edit: New url -- https://github.com/splinter-build/splinter)

81 files changed, 1869 insertions(+), 3094 deletions -- and that, of course, doesn't count lines that have changed multiple times over the patch series.

Ninja's code is a mess, and it's taking me substantially longer to understand and start making the changes I want compared with other projects that use more recent c++ standards.

Const-casts used in several places, "-1" assumed to be converted to max-int for unsigned types. C++17 data structures like StringPiece cobbled together and not even in a way that's api-similar to std::string_view. I don't even have the full list of complaints yet, I'm not done cleaning things up and modernizing the codebase to use C++17-isms.

So before I'll be able to adequately effect the changes I want in the Ninja code, I need to clean the code up. If I find any bugs, I'll report them, or provide patches, as appropriate, but the functionality changes that I'm looking for are probably not things that the Ninja project will merge (see previous discussions in this same comment thread), especially since they're mostly related to the boutique (and rather broken) build system I use at work , so I feel no need to confine myself to C++98.

I don't value the ability to run on old machines. It's not relevant to me, but being able to use the new features (language, and library alike) in C++11 / C++14 / C++17 is incredibly important to me. Since I'd rather keep up to date with code that lands in the official Ninja upstream, I want to find out what timeline the Ninja project has for adopting newer standards. The sooner that happens, the sooner my patches can start landing in Ninja proper, instead of my fork, and the less work I have to do to get my special case functionality working, or kept-working with upstream changes over time.

[jhasse]

I think it would be a good idea to set up CI to build with an old compiler, e.g. g++ from RHEL/CentOS 6.

If I understood nico correctly, the goal is not to touch working code if possible. That means when Ninja adopts C++11 in the future, it would only be for new code being added.

[jonesmz]

I think it would be a good idea to set up CI to build with an old compiler, e.g. g++ from RHEL/CentOS 6.

Fantastic. Glad to hear it.

If I understood nico correctly, the goal is not to touch working code if possible. That means when Ninja adopts C++11 in the future, it would only be for new code being added.

That's silly. If the code compiles with a newer standard, and the project accepts the use of that newer standard in PRs, then adjusting the entire codebase to the newer standard (slowly, and carefully, obviously) will enable benefits including, but not limited to:

• Better performance. Newer standards have new library features, such as std::string_view, that make it much easier to achieve better performance than doing things the old way
• Less eye-bleed. Potential contributors will expend less mental effort and eye strain to understand the codebase, and provide patches to improve things
• Less bugs via using standard implementations of various things (again, StringPiece vs string_view), via easier to read code (e.g. range-based for-loops), and via keywords designed to enhance language safety such as the override keyword, or the noreturn attribute, or various other things that compilers can emit warnings about.

I'd still like to know if Ninja will switch to C++03 on or before 2023. That's 20 years from the C++03 standard being published. A yes or no answer is perfectly fine, and I'll accept "no" as the answer without complaint.

[jhasse]

Better performance. Newer standards have new library features, such as std::string_view, that make it much easier to achieve better performance than doing things the old way

If benchmarks show a performance increase, I think that's something we'd want to merge :) But obviously quality-of-life stuff like nullptr won't change performance.

Less eye-bleed. Potential contributors will expend less mental effort and eye strain to understand the codebase, and provide patches to improve things

It's much more important for Ninja to be stable, reliable and to work. No great need for improvements when it's already doing so well ;)

Less bugs via using standard implementations of various things (again, StringPiece vs string_view), via easier to read code (e.g. range-based for-loops), and via keywords designed to enhance language safety such as the override keyword, or the noreturn attribute, or various other things that compilers can emit warnings about.

I agree with that one. But: You clutter up the Git history and sometimes, even trivial changes like adding the override keyword, can lead to mistakes.

I'd still like to know if Ninja will switch to C++03 on or before 2023. That's 20 years from the C++03 standard being published. A yes or no answer is perfectly fine, and I'll accept "no" as the answer without complaint.

As C++03 doesn't really add much (it's more just a bugfix release) and I don't think that we rely on any of those C++98 bugs, I'd would say that Ninja's codebase is already C++03. C++98 is often just used as a synonym for pre-C++11.

[jonesmz]

If benchmarks show a performance increase, I think that's something we'd want to merge :) But obviously quality-of-life stuff like nullptr won't change performance.

Well, nullptr can eliminate a type of bug with regards to the function chosen out of a set of overloads, so I don't really consider nullptr performance related, it's a safety feature.

But sure. Measurable performance increases are things Ninja wants to merge. I'll hold you to that for any PRs I open in the future.

It's much more important for Ninja to be stable, reliable and to work. No great need for improvements when it's already doing so well ;)

That's subjective, isn't it?

Personally I don't think it's appropriate to call a project "stable" that, for the last six months until this week, had over 200 issues and over 100 PRs.

If it were something as large as Firefox, then sure that's not a lot. But Ninja has approximately 15,000 lines of code, according to SLOCCount. I write that many lines of code every month or two for work. 100 PRs is really large.

The code not changing for that period of time isn't an indicator of stability. Perhaps we can discard the number of issues, as those can frequently be PEBKAC errors, but the PRs are quite a different story. People cared enough that they dug into the code to correct problems they were experiencing. Many of the PRs had comments to the effect of "I'm using a custom build with this patch because it doesn't work for my situation otherwise".

I'd also point out that a lot of the PRs change a lot of code, in overlapping ways. C++11 and newer allow for things like (for example) for-loops to be much more compact, reducing the size of patches, which can help sometimes with overlap and related code merging concepts. Obviously the magnitude of improvement is low in this example but not non-existent.

But I agree that not introducing new bugs into a project is an overwhelmingly good idea. Which is why I like the override and final keywords. They make it difficult to screw up inheritance.

I agree with that one. But: You clutter up the Git history and sometimes, even trivial changes like adding the override keyword, can lead to mistakes.

I agree with you in principal that trivial changes like adding the override keyword can somehow add a bug, but to be completely honest here I just can't see how a commit that only adds the override keyword can introduce a bug? Either the project continues to compile because the decorated function really does override a base class member function, or it doesn't?

Do you mean "No matter what change someone makes, there's always the possibility of a screwup"?

Or do you actually mean that the override keyword, specifically, can add a bug?

As C++03 doesn't really add much (it's more just a bugfix release) and I don't think that we rely on any of those C++98 bugs, I'd would say that Ninja's codebase is already C++03. C++98 is often just used as a synonym for pre-C++11.

Great :-)

So how about TR1, the additions to C++03 published in 2007?

If you're ok with TR1, you would have no objections to people using the headers \<tuple>, \<array>, \<unordered_map> and the other various things from the TR1 document that was the bulk of C++03 ?

I'd be happy to open a PR later today that replaces https://github.com/ninja-build/ninja/blob/master/src/hash_map.h with the C++03 / TR1 .

If I do that and the PR doesn't introduce performance or feature regressions caught by the existing test suite, would you accept it? Would you accept it within a few weeks?

(Edit, this is now a PR: https://github.com/ninja-build/ninja/pull/1696)

[jhasse]

Personally I don't think it's appropriate to call a project "stable" that, for the last six months until this week, had over 200 issues and over 100 PRs.

If it were something as large as Firefox, then sure that's not a lot. But Ninja has approximately 15,000 lines of code, according to SLOCCount. I write that many lines of code every month or two for work. 100 PRs is really large.

I disagree that this is an indicator that ninja isn't "stable". It's more of an indication that there was a lack of maintenance. That's exactly what I'm trying to improve in the coming weeks, starting by reducing the number of open PRs.

The code not changing for that period of time isn't an indicator of stability. Perhaps we can discard the number of issues, as those can frequently be PEBKAC errors, but the PRs are quite a different story. People cared enough that they dug into the code to correct problems they were experiencing. Many of the PRs had comments to the effect of "I'm using a custom build with this patch because it doesn't work for my situation otherwise".

I would also not say that this is a problem of stability, more a lack of features (or rather a disagreement on what should be part of Ninja and what shouldn't). As nico said a few posts earlier:

"Many pull requests ignore that blurb above (and the "Match the Ninja coding style (see below)") and they add stuff that's useful in some cases, but that also make ninja more complicated. And often what the patch wants to achieve can be achieved without changes to ninja in other ways." https://github.com/ninja-build/ninja/issues/1403#issuecomment-378923660

I'd also point out that a lot of the PRs change a lot of code, in overlapping ways. C++11 and newer allow for things like (for example) for-loops to be much more compact, reducing the size of patches, which can help sometimes with overlap and related code merging concepts. Obviously the magnitude of improvement is low in this example but not non-existent.

Hm ... isn't the improvement for for-loops really non-existent? It only changes one line (the line is shorter, yes, but that doesn't help diffs).

But I agree that not introducing new bugs into a project is an overwhelmingly good idea. Which is why I like the override and final keywords. They make it difficult to screw up inheritance.

Their advantage is greater, if you happen to refactor your code a lot, which isn't something Ninja wants to do.

I agree with you in principal that trivial changes like adding the override keyword can somehow add a bug, but to be completely honest here I just can't see how a commit that only adds the override keyword can introduce a bug? Either the project continues to compile because the decorated function really does override a base class member function, or it doesn't?

Yeah it's very unlikely for the override keyword. For-each loops or auto are better examples.

For override the Git-history argument still stands though.

Do you mean "No matter what change someone makes, there's always the possibility of a screwup"?

Or do you actually mean that the override keyword, specifically, can add a bug?

Hm I don't know ... override really was a bad example, sorry.

So how about TR1, the additions to C++03 published in 2007?

IF you're ok with TR1, you would have no objections to people using the headers , , and the other various things from the TR1 document that was the bulk of C++03 ?

I'd be happy to open a PR later today that replaces https://github.com/ninja-build/ninja/blob/master/src/hash_map.h with the C++03 / TR1 .

If I do that and the PR doesn't introduce performance or feature regressions caught by the existing test suite, would you accept it? Would you accept it within a few weeks?

Before you do that, please wait until

• Ninja 1.9.0 is released / branched off the master branch,
• I've managed to set-up CI with an older compiler,
• I've added a macOS build to Travis CI.

Then the PR would need to actually improve performance or fix an existing bug, and pass CI without any workarounds / hacks / #ifdef madness.

[jonesmz]

I disagree that this is an indicator that ninja isn't "stable". It's more of an indication that there was a lack of maintenance. That's exactly what I'm trying to improve in the coming weeks, starting by reducing the number of open PRs.

Btw, I was really excited to watch all the work you've done the last two weeks. I used ninja at work, and have my own forked version that have a lot of customization related to our legacy build system. I'm really glad Ninja is getting some developer velocity again.

I would also not say that this is a problem of stability, more a lack of features (or rather a disagreement on what should be part of Ninja and what shouldn't). As nico said a few posts earlier:

"Many pull requests ignore that blurb above (and the "Match the Ninja coding style (see below)") and they add stuff that's useful in some cases, but that also make ninja more complicated. And often what the patch wants to achieve can be achieved without changes to ninja in other ways." #1403 (comment)

I mean, I disagree with Nico's comment, but I respect that it's a matter of personal opinion so I can't really justify trying to change his mind more than I already did. Ultimately everyone's got their own set of personal values shrug.

Hm ... isn't the improvement for for-loops really non-existent? It only changes one line (the line is shorter, yes, but that doesn't help diffs).

• Reduction in number of variables.
• Reduction in the number of places that use operator-> instead of operator*, and then (what would be called, if it had a name) operator. against a reference-to-the-thing, instead of an iterator-to-the-thing. Which can have performance implications in some situations, and also makes it less likely for a programmer to introduce a use-after-invalidation on the iterator. Typically this only becomes a problem in really really large for-loops.
• Normalizes the appearance of for-loops, allowing developers much faster brain-cache access to what a particular set of symbols in the code represents.
• Some for-loops in the ninja codebase are unnecessarily complex. E.g. storing iterators before the line in question, and then looping. These can all be normalized, providing a small reduction in the probability that two patches might touch the same line.

I wasn't trying to say that every for loop is going to be touched by more than one patch. Certainly not. I was simply trying to point out that there's a marginal benefit with regard to code-churn, since C++11 allows for more compact representations of the same semantic idea. Not just for for-loops, but for-loops are a great example.

Does that mean that if every for loop that can be a range-based for loop was converted, that any particular two open PRs will have a reduction in overlap? No.

It just means that it's possible, even if it's unlikely.

Btw, you can see what the ninja codebase would look like with range-based for-loops here: https://github.com/jonesmz/splinter/tree/3-ranged-for-loops (edit, new URL: https://github.com/splinter-build/splinter/tree/3-ranged-for-loops)

But I agree that not introducing new bugs into a project is an overwhelmingly good idea. Which is why I like the override and final keywords. They make it difficult to screw up inheritance.

Their advantage is greater, if you happen to refactor your code a lot, which isn't something Ninja wants to do.

Sure, more refactoring results in more opportunities for the override keyword to be useful.

But there are more stakeholders to the Ninja codebase than the folks who have direct repository access to https://github.com/ninja-build/ninja.

It's fine if you want to say that you're not interested in those stakeholders, and that's certainly your prerogative. But if you added the override keyword to every appropriate place in the Ninja codebase, it would make anyone who opened a PR's life easier, because they would be able to recognize when a change broke assumptions about how the inheritance hierarchy is intended to function.

It would allow for people who are hacking on Ninja to get faster feedback of problems in their code (compiler errors, instead of test application errors), increase the quality of PRs that are opened (because surely not every PR passes unit tests, eh?), and reducing the number of PRs that fail on continuous integration.

There's also the benefit to people who maintain their own set of patches. There's a lot of folks who commented that they did this in various comments here on github.

For override the Git-history argument still stands though.

Sorry, how?

I've added the override keyword to every place it was justified in my fork (and the final keyword, as well). It's a single commit. Roughly 100 lines of code change. https://github.com/jonesmz/splinter/tree/7-override-final-keywords

What's the threshold for (paraphrasing) "cluttering the git history" ? A single commit? More?

Before you do that, please wait until

• Ninja 1.9.0 is released / branched off the master branch,
• I've managed to set-up CI with an older compiler,
• I've added a macOS build to Travis CI.

Sure, I can wait. I already have the change, I just have to rebase it on the Ninja codebase, instead of my fork.

https://github.com/jonesmz/splinter/tree/5-unordered_map (edit, the PR: https://github.com/ninja-build/ninja/pull/1696)

Then the PR would need to actually improve performance or fix an existing bug, and pass CI without any workarounds / hacks / #ifdef madness.

Can you document this?

Hacking.md says

Github pull requests are convenient for me to merge (I can just click a button and it's all handled server-side), but I'm also comfortable accepting pre-github git patches (via send-email etc.).

Good pull requests have all of these attributes:

Are scoped to one specific issue
Include a test to demonstrate their correctness
Update the docs where relevant
Match the Ninja coding style (see below)
Don't include a mess of "oops, fix typo" commits
These are typically merged without hesitation. If a change is lacking any of the above I usually will ask you to fix it, though there are obvious exceptions (fixing typos in comments don't need tests).

I am very wary of changes that increase the complexity of Ninja (in particular, new build file syntax or command-line flags) or increase the maintenance burden of Ninja. Ninja is already successfully used by hundreds of developers for large projects and it already achieves (most of) the goals I set out for it to do. It's probably best to discuss new feature ideas on the mailing list before I shoot down your patch.

I don't feel that this set of guidelines are accurate anymore.

A professor I had in college once said:

All programs have bugs. If everything else about two programs are equal (e.g. same performance, features, bugs, so on) the smaller of the two is the superior. Therefore, the best possible program is zero lines of code and has at least one bug.

If you don't want patches that don't cause measurable performance improvements, or bugs that someone with repository access agrees is a bug, then that completely precludes changes that are for the sake of improving developer quality of life.

The requirement of making a measurable performance improvement, would also preclude someone making a change to replace the StringPiece class (with c++11 string view, sometime in the far future), which has some weird conventions with regards to const-casting in the codebase.

And that's fine, it's your project, and I'll do my own thing in my own version of Ninja to suit my own needs. But please document that that's the case. I don't feel that match the Ninja coding style explains this sufficiently.

Or, do you mean, measurably not reduce performance? That's a very different concept than a requirement to improve performance. E.g. >= vs >.

[jonesmz]

Also, while I'm at it...

How is it OK to require any newly added files have copyright Google Inc. ? If I submitted a PR that adds a new file, and I don't claim that Google owns the copyright for that file, will the PR be rejected?

I think Hacking.md really needs to be revisited. It seems to me like you need a contributor license agreement if you expect people to assign the copyright for their work to Google.

See the last line of the quote below.

Coding guidelines
Generally it's the Google C++ coding style, but in brief:

Function name are camelcase.
Member methods are camelcase, except for trivial getters which are underscore separated.
Local variables are underscore separated.
Member variables are underscore separated and suffixed by an extra underscore.
Two spaces indentation.
Opening braces is at the end of line.
Lines are 80 columns maximum.
All source files should have the Google Inc. license header.

[jhasse]

Reduction in the number of places that use operator-> instead of operator*, and then (what would be called, if it had a name) operator. against a reference-to-the-thing, instead of an iterator-to-the-thing. Which can have performance implications in some situations [...]

But those would also be situation where you can't immediately see if the change doesn't introduce any new bugs.

There's also the benefit to people who maintain their own set of patches. There's a lot of folks who commented that they did this in various comments here on github.

In the long run, yes. But right now such a change would result in a lot of conflicts for these people as they have based their patches on non-C++11 Ninja as it is right now.

Sorry, how?

[...]

What's the threshold for (paraphrasing) "cluttering the git history" ? A single commit? More?

Sorry I've should had made that more clear: I meant the line Git history you get when using git blame for example.

If you don't want patches that don't cause measurable performance improvements, or bugs that someone with repository access agrees is a bug, then that completely precludes changes that are for the sake of improving developer quality of life.

Let's put it this way: Improving developer quality of life is something worth considering, but above anything else - the most important thing for ninja at this stage of its life - is to be stable and not introduce any regressions. Yes, that could definitely be emphasized more in HACKING.md, but I think it's there in this piece:

"I am very wary of changes that increase the complexity of Ninja (in particular, new build file syntax or command-line flags) or increase the maintenance burden of Ninja. Ninja is already successfully used by hundreds of developers for large projects and it already achieves (most of) the goals I set out for it to do. It's probably best to discuss new feature ideas on the mailing list before I shoot down your patch."

with c++11 string view, sometime in the far future

std::string_view is C++17 btw.

Or, do you mean, measurably not reduce performance? That's a very different concept than a requirement to improve performance. E.g. >= vs >.

>

[jonesmz]

But those would also be situation where you can't immediately see if the change doesn't introduce any new bugs.

True.

In the long run, yes. But right now such a change would result in a lot of conflicts for these people as they have based their patches on non-C++11 Ninja as it is right now.

Also true.

Sorry I've should had made that more clear: I meant the line Git history you get when using git blame for example.

Ok. That's more clear. I'm not sure I see the problem, but I'm glad I know more clearly what you meant.

Let's put it this way: Improving developer quality of life is something worth considering, but above anything else - the most important thing for ninja at this stage of its life - is to be stable and not introduce any regressions. Yes, that could definitely be emphasized more in HACKING.md, but I think it's there in this piece:

"I am very wary of changes that increase the complexity of Ninja (in particular, new build file syntax or command-line flags) or increase the maintenance burden of Ninja. Ninja is already successfully used by hundreds of developers for large projects and it already achieves (most of) the goals I set out for it to do. It's probably best to discuss new feature ideas on the mailing list before I shoot down your patch."

Ok. I mean, as I think you can tell, I still disagree with you, but I respect that this is a difference of opinion and values, so there's not much I can do to change that opinion beyond what I've already done, so I'm not going to push the topic any harder.

std::string_view is C++17 btw.

Of course. My mistake.

Or, do you mean, measurably not reduce performance? That's a very different concept than a requirement to improve performance. E.g. >= vs >.

>

I believe that this is an unreasonable position to have.

Requiring monotonic improvements to performance, such that any change must strictly increase performance (not retain the same performance) makes it very likely that you'll be stuck in a local maxima, instead of a global maxima.

It further precludes a very wide range of potential contributions. This one, for example : https://github.com/ninja-build/ninja/pull/1458

Could you provide more elaboration for this?

And separate from that, could you please include this in the hacking.md information?

[jhasse]

Requiring monotonic improvements to performance, such that any change must strictly increase performance (not retain the same performance) makes it very likely that you'll be stuck in a local maxima, instead of a global maxima.

Multiple commits can be bundled in a PR, so not every change must strictly increase performance. It may even decrease it. But all in all a PR should improve Ninja, wouldn't you agree? Ninja's focus is speed, so I don't see what so unreasonable about requiring PRs not to change that. Speed is something Ninja's users expect from it, so decreased performance is a regression. A bug. PRs should fix bugs (or sometimes even add new features), not introduce them.

It further precludes a very wide range of potential contributions. This one, for example : #1458

As you can see from the PR, it broke CI, since CI generates lexer.cc from lexer.cc.in. What would have happened, if CI didn't generate it, but used the fallback from Git? I would have probably merged the PR, breaking compilation of the master branch for many people.

So it's a great example that even the simplest change can cause bugs.

[jonesmz]

Multiple commits can be bundled in a PR, so not every change must strictly increase performance. It may even decrease it. But all in all a PR should improve Ninja, wouldn't you agree?

No, I don't agree, actually, not in the way I think you mean here.

"Improvement" is not simply "Number of actions ninja does per second goes up". Improvement in terms of end-user behavior may involve speed, may involve features, bug fixes, quality-of-life tweaks, and all sorts of things. Improvement in terms of developers can include language features, code cleanliness, const-correctness, tooling such as automatic code-formatting, additional unit tests, and all sorts of other things.

Sometimes you can improve something in one fashion, but at the cost of making another aspect worse. It's a tradeoff.

You've stated that you're not willing to compromise on speed, so much so that you're only willing to accept PRs that improve the speed, even if the PR otherwise improves other aspects of Ninja with no downsides.

Personally, I suspect that maybe we were talking past each other.

Did you really mean that only PRs that improve speed, regardless of other considerations would be accepted?

Or did you mean that only PRs that do not make Ninja slower would be accepted? I think this is probably what you meant?

Ninja's focus is speed, so I don't see what so unreasonable about requiring PRs not to change that.

Er, well, but you said that you expect all PRs to improve speed. I asked to clarify if you meant all PRs not make things slower, and I thought you responded back that you wanted all PRs to show an improvement in speed, not just the same speed.

Speed is something Ninja's users expect from it, so decreased performance is a regression. A bug. PRs should fix bugs (or sometimes even add new features), not introduce them.

Ehhhh, many Ninja users expect speed. For my purposes, Ninja's ability to parse and start executing a build graph is already so incredibly faster than what I was using before, we could literally make ninja 10 times slower and I wouldn't even notice. I'd even say I might not notice if we slowed ninja down by 100x, but I can't say that for certain. Regardless, I think we've already reached the point where additional speed is not meaningfully useful to many ninja users. Perhaps there are many users who still want more speed, and perhaps even most of the users still want more speed, but I wager there's at least a sizable minority that thinks Ninja is already more than fast enough and would rather see other improvements than simply more speed.

What I want is more features. For what I use Ninja for, the biggest improvement would be for Ninja to coordinate with other Ninja processes to not exceed system limits. That would save me considerable re-work on my higher-level build system code (legacy garbage, unfortunately).

Since I'm willing to put my effort where my mouth is, I'm engaging with the Ninja community (e.g., this conversation that we're having. Thank you for talking with me), and working on code (e.g., that fork I have on github that I put several links for in this comment thread).

What I'm interested in is a codebase that is easy to reason about, so that I can customize it to suit my needs. Since I strongly believe that the vast majority of programmers like the type of code improvements (Again, assuming they don't break things, cause bugs, make things slower, etc.) I wanted to make in the Ninja codebase, I'm doing that work on Github.

Nico doesn't like the first patch I made available, and it's a hard-prerequisite for the others, so that's all there is to it. I'll do things in my world, pull improvements you make, and report issues to you as I find them. But I'm not able to provide PRs because none of them meet the criteria that this project apparently wants. That's cool. You do you and all. I just don't think it's the most effective strategy.

It further precludes a very wide range of potential contributions. This one, for example : #1458

As you can see from the PR, it broke CI, since CI generates lexer.cc from lexer.cc.in. What would have happened, if CI didn't generate it, but used the fallback from Git? I would have probably merged the PR, breaking compilation of the master branch for many people.

So it's a great example that even the simplest change can cause bugs.

I think that perhaps you're missing the forest for the trees with regard to my example.

If we assume that the original contributor fixes the PR such that lexer.cc/lexer.cc.in are set up correctly, then that PR still fails to demonstrate improved performance (As in, there's no documentation in the PR on if the performance improves). So with the requirement that all PRs increase performance, and PRs that neither decrease nor increase performance aren't acceptable, that PR can't be merged.

In my personal experience (As in, you might have a different experience than me on this), the vast majority of C / C++ programmers believe that const-correctness is desirable. Frequently, it's the very first thing I ever try to do with a new codebase that I encounter, and going through the exercise of improving const-correctness for a codebase regularly finds numerous issues.

I certainly was not advocating that you should accept PRs that introduce problems. I was merely pointing out that const-correctness is desirable versus not-const-correctness, and also that const-correctness is generally not thought of as having a performance improvement.

Regardless, I still think you should include these requirements in your Hacking.md file. If you like, I'd be willing to write a draft and open a PR.

Have you also considered putting a check in your CI tool on the performance? If you can put in some kind of hard cutoff, such that the CI fails if the performance of the build is below that threshold, you'll have an easier time verifying pull requests. If you don't already have something to do that, I would also be willing to write code/script to do so for you. Just let me know.

[jhasse]

You've stated that you're not willing to compromise on speed, so much so that you're only willing to accept PRs that improve the speed, even if the PR otherwise improves other aspects of Ninja with no downsides.

No, that's not what I said (and not the case). Here's what I said (notice the emphasize):

Then the PR would need to actually improve performance or fix an existing bug, and pass CI without any workarounds / hacks / #ifdef madness.

Also I was talking about your PR which replaces hash_map with unordered_map in particular.

If you aren't certain how I meant something, please just ask and wait for my response before stating other things which depend on this. That way we can avoid these lengthy posts which I'm sure are annoying to anyone who wants to follow along.

Have you also considered putting a check in your CI tool on the performance?

Yes, but it's very hard to do, as performance of the CI runners isn't steady or reliable. Might not even be a real world scenario in some cases.

[jonesmz]

You've stated that you're not willing to compromise on speed, so much so that you're only willing to accept PRs that improve the speed, even if the PR otherwise improves other aspects of Ninja with no downsides.

No, that's not what I said (and not the case). Here's what I said (notice the emphasize):

Then the PR would need to actually improve performance or fix an existing bug, and pass CI without any workarounds / hacks / #ifdef madness.

I misread. Apologies.

[rprichard]

FWIW: CentOS/RHEL 6/7 have a "devtoolset" package that provides a modern C++ toolchain (currently C++17). I haven't used it myself though.

https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/7/html/7.0_release_notes/dts7.0_release

https://www.reddit.com/r/cpp/comments/86juhc/devtoolset_is_a_game_changer_for_c_development_on/

[ryandesign]

There are no plans on adopting C++11.

Alas, ninja 1.12.0 requires C++11. See #2089.