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

Fix for issue #619 #656

Merged
merged 2 commits into from
May 24, 2017
Merged

Fix for issue #619 #656

merged 2 commits into from
May 24, 2017

Conversation

mihaibudiu
Copy link
Contributor

I am not sure this is the best way to do this, but it seems to work.

Copy link
Contributor

@sethfowler sethfowler left a comment

Choose a reason for hiding this comment

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

LGTM. Could you please update .travis.yml to remove the line that runs make cpplint? Otherwise we'll run cpplint twice on Travis.

Makefile.am Outdated
@@ -154,6 +154,8 @@ cpplint_FILES += $(noinst_HEADERS)
clean-local:
-rm -f $(BUILT_SOURCES) $(CLEANFILES)

check: cpplint

Copy link
Contributor

@ChrisDodd ChrisDodd May 24, 2017

Choose a reason for hiding this comment

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

Is this correct, or should it be check-local: ? I'm never sure with automake. Possibly also redirect stdout of cpplint to avoid the spew of "processing x" messages that are mostly uninteresting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stupid cpplint writes the message on stderror.
Want me to change the python file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: check-local, I wondered that myself. It does seem more idiomatic, so perhaps that by itself is an argument in its favor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a --quiet option to cpplint

Copy link
Contributor

Choose a reason for hiding this comment

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

The only difference I could find is that if you use check-local and -j it will run the cpplint check in parallel with other checks, while if you use check it will not (and will bail out if cpplint fails, before running any tests).

@mihaibudiu mihaibudiu merged commit 8cf0913 into p4lang:master May 24, 2017
@mihaibudiu mihaibudiu deleted the issue619 branch May 24, 2017 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants