The false-positive was caused by an error in matching <...> after
unique_ptr. The old expression essentially said:
<.*?>\(
The "?", making "*" non-greedy, was meant to prevent ".*" from
matching more than one <...> block.
But that does not work -- . would still match the '>' which ended the
first block and carry on until an '>(' was found, if necessary.
So in the concrete example:
std::unique_ptr<T> result = std::make_unique<T>(
what the presubmit check "though" was the the type parameter was
"T> result = std::make_unique<T", instead of just "T".
A better way to ensure matching just one <...> block was already used
in the sibling check pattern (null_construct_pattern) -- splitting the
(possibly nested) <...> block into the part containing <s and the part
containing >s (note that a regular expression cannot check that both
parts have a matching number of < and >).
This CL just adopted that better technique for the faulty pattern.
Also remove a temporary hack from webstore_provider.cc added to bypass
the failing check.
Bug: 733662, 827961
Change-Id: Ia70cc4333f8afc4d45b1f676ea1bc870f6a3a079
Reviewed-on: https://chromium-review.googlesource.com/998194
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548896}
_CheckUniquePtr consistes of two checks -- one related to nullptr, one
related to make_unique. The error messages are currently swapped by
accident. This CL fixes that and adds a check for that.
It also removes some confusing "java/src" subpaths from the mock file
paths in the check. Those got in via copy&paste and are confusing,
while not relevant for what the test is testing.
Bug: 827961
Change-Id: Iaf3272cfc5fe0a641feaee861ad17fe71505800b
Reviewed-on: https://chromium-review.googlesource.com/999604
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548807}
Currently, _CheckUniquePtr PRESUBMIT check reports each failing line as
a separate error. This repeats the error explanation and thus clutters
the output necessarily.
This CL makes _CheckUniquePtr collect all occurences of the two issues
it checks for (direct use of unique_ptr constructor and replaceability
with nullptr) and group them under a separate single error, one for each
of the both types of check.
It also adds the failing line into the output, to make it easier to
understand the issue already from the presubmit logs.
This follows what is done for other checks, e.g., _CheckNoPragmaOnce).
Bug: 827961
Change-Id: Ic7d60a05b6f96da741f1401422f4a1690bb6e279
Reviewed-on: https://chromium-review.googlesource.com/990132
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548081}
The presubmit check _CheckUniquePtr guards against calling
std::unique_ptr constructor directly, instead directing code authors to
use std::make_unique.
This check currently fails to match multiline expressions. While it
catches
bar = std::unique_ptr<T>(foo);
it does not catch
bar =
std::unique_ptr<T>(foo);
nor does it catch
bar = std::unique_ptr<T>(
foo);
This CL fixes it by extending the match pattern to catch all lines with
the substring "std::unique_ptr<T>(". (But not those with
"std::unique_ptr<T>()", which should be handled by the "nullptr-check".)
Bug: 827961
Change-Id: I376b5e9811418205e294e97de0b6b7bcbf6891d2
Reviewed-on: https://chromium-review.googlesource.com/989735
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548045}
The PRESUBMIT test _CheckUniquePtr reported the following
false-positive:
std::unique_ptr<ParseResult> result = std::make_unique<ParseResult>();
suggesting that one uses nullptr instead.
This CL fixes that bug and also adds regression tests for this
presubmit check.
Bug: 827961
Change-Id: I60a088f6590f01c51be7e3ffc0c6d65ad9e5c329
Reviewed-on: https://chromium-review.googlesource.com/989972
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547635}
For (Objective-)C++, there is
_CheckNoProductionCodeUsingTestOnlyFunctions in //PRESUBMIT.py which
can detect when a method with a ForTesting suffix is referenced in
production code.
This bug tracks adding such similar presubmit check for production
.java files, which are approximated as .java files with no "test"
or "junit" in the path and filename.
The check is a modified copy of the one for C++. It does not attempt
to share code with the C++ check, because some details are simpler for
Java (e.g., no need to consider *_for_testing as well). The check is
just a presubmit prompt, not error, because there are false positives
to be expected.
Bug: 821981
Change-Id: I3bb137197070ac696fbe0fd35d50abbfb823a8d8
Reviewed-on: https://chromium-review.googlesource.com/977581
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546063}
Currently, _CheckUmaHistogramChanges detects use of undefined
histogram names in (Objective-)C++ files.
This CL extends that support to Java as well.
Bug: 821981
Change-Id: Ida931db191f0793927b0e957e64bf6dac699d502
Reviewed-on: https://chromium-review.googlesource.com/978163
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545678}
Currently, _CheckUmaHistogramChanges detects the cases like calling
UMA_HISTOGRAM_BOOLEAN("NonexistingHistogram.Typo", true)
when "NonexistingHistogram.Typo" is not a histogram name defined in
histograms.xml.
However, it won't detect the case when the name is after a line break:
UMA_HISTOGRAM_BOOLEAN(
"NonexistingHistogram.VeeeeeeeryLoooooooongName.WithSubitems",
true)
This will be often the case once the check gets extended to Java,
where the UMA_HISTOGRAM* macros are replaced with the
RecordHistogram.record*Histogram methods, which have longer names.
Bug: 821981
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I71d01f3b7012e8a8d6c4628d67a470c57005cd56
Reviewed-on: https://chromium-review.googlesource.com/978219
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545676}
The _CheckUmaHistogramChanges checks whether newly added
UMA_HISTOGRAM_* calls only use defined histogram names.
This CL removes two ways to introduce false positives:
* Unrelated macro names which contain UMA_HISTOGRAM as a continuous
substring
* More than one string literal on the line with the histogram name.
While those cases are rare, there seems no downside to fixing them.
Bug: 821981
Change-Id: Ie1a803f562883f567d577e742ed2ed87fd0dfe66
Reviewed-on: https://chromium-review.googlesource.com/978245
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545583}
The root PRESUBMIT.py contains
_CheckNoProductionCodeUsingTestOnlyFunctions which checks against
including a call to a 'for testing only' method in production code.
That check has not been tested. This CL adds two tests for it.
Additionally, the CL also fixes two issues in the presubmit tests:
* FilterSourceFile in PRESUBMIT_test_mocks.py did not match the
production version from tools/depot_tools/presubmit_support.py: the
production requires the filename to be in the whitelist, while the
mock version was OK with just not being in the blacklist. The CL
modifies the mock version to match the production.
* The test for _CheckAndroidTestJUnitInheritance did not adhere to
the whitelisted filename pattern (*Test.java). This CL changes the
names of the fake files to match the pattern.
Bug: 821981
Change-Id: I65edf07ddb2ae26ad7d08ceb7cf4d51b482b5e56
Reviewed-on: https://chromium-review.googlesource.com/966605
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543783}
Missing include guards tend to break jumbo builds and makes
everyone involved sad so better to have a presubmit check for it.
There is a risk that this triggers on correct code so it includes
a way to disable it per source file. Include the string
"no-include-guard-because-multiply-included" and there won't
be any warning for that file.
By popular demand there will also be a warning if the name of
the include guard doesn't follow the coding standards, but since
mistakes are common, that check is only enabled for new files.
Bug: 814776
Change-Id: Id18b3d43288b9a064e537460d667957b355bbd33
Reviewed-on: https://chromium-review.googlesource.com/931761
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540526}
The testing::Test fixture (used by TEST macro) does not drain the
autorelease pool after a test. PlatformTest should be used.
Add a PRESUBMIT check that neither TEST nor testing::Test is used
in iOS Objective-C++ test files. Files are assumed to be iOS if
either their base name match '\bios\b' or one of the component in
the path is 'ios'.
Expand MockInputApi to filter files in mocks of AffectedFiles and
AffectedSourceFiles function, adding missing mocked functions too.
Fix unit tests that were failing after the filtering is correctly
implemented.
Bug: none
Change-Id: I0af99b6658b8e15888dfcfb94345eb879ab9fd37
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/937204
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539829}
This CL admits the TODO(crbug.com/...) format for bug
references in the code base and avoids the presubmit
warning in those cases. This pattern is pervasive and
even necessary in some folders (e.g. ios/) so, although
it isn't linkified by cs.chromium.org, it's accepted.
Change-Id: Ifb6eda7967271acf76ddfb5fa555ed60f9bce415
Reviewed-on: https://chromium-review.googlesource.com/833203
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525052}
Our code search doesn't linkify crbug[.com] if they are
not prefixed with http[s]:// (e.g. [1]). This CL adds a
warning on upload that this is happening and suggests
prefixing https://, since it seems like we are not going
to be able to linkify them automatically in CS anytime
soon (see bug).
It looks a bit like:
** Presubmit Warnings **
Found unprefixed crbug.com URL(s), consider prepending https://
ui/ozone/platform/drm/gpu/gbm_buffer.h:107 // TODO(mcasas): crbug.com
ui/ozone/platform/drm/gpu/gbm_buffer.h:108 // TODO(mcasas): crbug/123
[1] https://cs.chromium.org/chromium/src/BUILD.gn?type=cs&q=crbug.com&sq=package:chromium&l=65
Bug: 762061
Change-Id: Ib58bc6b58dfa61a7bf421b0ed55184705cee767c
Reviewed-on: https://chromium-review.googlesource.com/822973
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524073}
It's the same thing as linux_android_rel_ng without the tests.
Note that there's no test-less mirror of android_n5x_swarming_rel,
which is also on the CQ.
Need to remove it here before updating buildbucket.config
TBR=phajdan.jr@chromium.org
Bug: 773872
Change-Id: I7a04f92f19752ab3f31cd2203249f485fe678c94
Reviewed-on: https://chromium-review.googlesource.com/748216
Commit-Queue: Benjamin Pastene <bpastene@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513223}
These presubmit warnings prevent people from adding new JUnit3 tests or
use inheritance in JUnit4 testing, it also prevent people from using
the deprecated JUnit framework.
For more on JUnit4 migration, please check
//testing/android/docs/junit4.md
Bug: 640116
Change-Id: I941b595f6fbc01ac60a2647ab0af64482596d9cc
Reviewed-on: https://chromium-review.googlesource.com/634603
Commit-Queue: Yoland Yan <yolandyan@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497790}
Since mojo manifests don't have a consistent naming convention, this
simply looks for JSON files with the "interface_provider_specs" key:
there are some manifests that don't specify this, but the important
part for security review is auditing what's exposed between processes.
Bug: 695922
Change-Id: Id30dae51ecc0cbfa35650ead14ef2dfd081c23d7
Reviewed-on: https://chromium-review.googlesource.com/621707
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497410}
This CL was reverted because it had a bug making it catch all changes to C++
files in the directories it applied to (I thought the fact that I had
successfully uploaded the CL itself verified the rule wouldn't trigger on
unrelated changes...but PRESUBMIT.py is not a C++ file). I have fixed the bug in
this version.
Original CL:
https://codereview.chromium.org/2900173003
Revert:
https://codereview.chromium.org/2897383002
Original description:
Relative header includes (#include path containing "../") can be used to cheat
the dependency system because they're not checked properly. This CL adds a
presubmit rule to catch these.
This rules applies to third_party/WebKit, but not anywhere else in third_party/.
There's one currently existing file I know of that would fail this rule:
ppapi/lib/gl/include/GLES2/gl2.h
I did not change this file when cleaning up the other headers since it appears
to be code imported from a third-party repo. I guess whoever updates this file
will have to bypass the rule.
BUG=724264
Review-Url: https://codereview.chromium.org/2900253003
Cr-Commit-Position: refs/heads/master@{#475587}
As a bonus, less regex than before and also correctly handles the
'!' prefix in DEPS files now.
Bug: 702851
Change-Id: Ic086cf0984bd96ba429dfdcac7dcce53616eab2d
Reviewed-on: https://chromium-review.googlesource.com/476026
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#464648}
As a bonus, less regex than before and also correctly handles the
'!' prefix in DEPS files now.
BUG=702851
Review-Url: https://codereview.chromium.org/2759593003
Cr-Commit-Position: refs/heads/master@{#458928}
See bug for details. tl;dr: we should not use direct links like
support.google.com/chrome/answer/123456
For now this is an ignoreable upload-only prompt.
BUG=679462
Review-Url: https://codereview.chromium.org/2627023003
Cr-Commit-Position: refs/heads/master@{#443255}
Skip presubmit warning when the introduced useless forward
declaration is made in third_party (with the exception of
blink). Take the opportunity to fix one test and style.
BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest
Review-Url: https://codereview.chromium.org/2568473002
Cr-Commit-Position: refs/heads/master@{#437833}
Checks that added or removed lines in affected header files
do not lead to new useless class or struct forward declaration.
BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest
Review-Url: https://codereview.chromium.org/2532583002
Cr-Commit-Position: refs/heads/master@{#434449}
Checks that affected header files do not contain useless class
or struct forward declaration.
BUG=662195
TEST=PRESUBMIT_test.py ForwardDeclarationTest
Review-Url: https://codereview.chromium.org/2525263002
Cr-Commit-Position: refs/heads/master@{#434329}
This removes references to DrMemory for MB, GN and buildbot, and also
removes references to redundant DrMemory bots and DrMemory targets.
BUG=655521, 644217
Review-Url: https://codereview.chromium.org/2455463002
Cr-Commit-Position: refs/heads/master@{#430388}
On Windows this type logs to the Event Log and on POSIX systems it logs to the messages log (or its equvalent).
As a side effect of adding the presubmit check for it this
CL fixes running the presumbit checks tests on Windows.
BUG=642115
Review-Url: https://codereview.chromium.org/2296783002
Cr-Commit-Position: refs/heads/master@{#419758}
Now unconditionally including some directory trees to capture imports that differ based on python version (at least those that have come up so far).
BUG=589318, 599692
Review URL: https://codereview.chromium.org/1846103005
Cr-Commit-Position: refs/heads/master@{#384907}
The bot has been removed from the memory FYI waterfall.
BUG=593920
Review URL: https://codereview.chromium.org/1844493002
Cr-Commit-Position: refs/heads/master@{#384106}
Now with check disabled for non-android checkouts.
This is required for any test that uses an .isolate to push files to the
device (e.g. base_unittests).
BUG=589318
Review URL: https://codereview.chromium.org/1840113002
Cr-Commit-Position: refs/heads/master@{#384039}
This is required for any test that uses an .isolate to push files to the
device (e.g. base_unittests).
BUG=589318
Review URL: https://codereview.chromium.org/1784373002
Cr-Commit-Position: refs/heads/master@{#383127}
This renames the macro to be more standard and express its intent and
relationship to DISALLOW_COPY_AND_ASSIGN. It renames the macro to
DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND, as it does what
the old DISALLOW_COPY_AND_ASSIGN does, but also whitelists the type
for Bind/Callback to try move it.
R=Nico
TBR=sky
BUG=561749
Review URL: https://codereview.chromium.org/1501793003
Cr-Commit-Position: refs/heads/master@{#363712}
