Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop namespace indents. #138

Merged
merged 1 commit into from
Mar 24, 2016
Merged

Conversation

davidzchen
Copy link
Contributor

As discussed offline, this change drops the indent within namespaces. This allows us to save
on horizontal whitespace and makes the code a bit easier to follow.

This change drops the indent within namespaces. This allows us to save
on horizontal whitespace and makes the code a bit easier to follow.
@mikedanese
Copy link
Contributor

have we discussed standardizing on a .clang-format before?

@davidzchen
Copy link
Contributor Author

I don't think we have, but I also think it is a good idea to standardize on an established style guide and .clang-format, especially since we are getting more contributors. I think we should consider adopting the Google C++ Style Guide to both be consistent with most Google open source projects.

@sparkprime
Copy link
Contributor

Nope, the Google C++ style guide is terrible. However I am definitely in favor of codifying the existing style and fixing any inconsistencies within.

@davidzchen
Copy link
Contributor Author

Why do you say that it is terrible? Another option is the LLVM style guide, which is in fact quite similar.


/** The captured self variable, or nullptr if there was none. \see CallFrame. */
HeapObject *self;
/** Used in error tracebacks. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like Github diffing is broken here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looks that way. This patch is purely whitespace changes only aside from the additions of the // namespace comments to the closing braces for the namespaces.

@sparkprime
Copy link
Contributor

LGTM

@sparkprime
Copy link
Contributor

A good rant on the style guide here: https://www.linkedin.com/pulse/20140503193653-3046051-why-google-style-guide-for-c-is-a-deal-breaker

I cut my teeth on Ogre graphics engine and Bullet physics engine, so the style I use is similar to theirs. It's also similar to the code I wrote at IBM for the X10 project.

@davidzchen
Copy link
Contributor Author

Yes, I have seen the rant, and I do not 100% agree with it. For example, it mentions that disallowing multiple inheritance is 100% a deal breaker, but I am not convinced that multiple inheritance is a "strength" in practice except for multiple interface inheritance.

In any case, let's move this discussion to a separate bug. I've opened #139 to discuss this.

@davidzchen davidzchen merged commit 334a225 into google:master Mar 24, 2016
sbarzowski pushed a commit to sbarzowski/jsonnet that referenced this pull request Jun 10, 2024
* Feature-complete commandline interface
* Make errors match cpp implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants