Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2589923002:
Add a hardcoded list of methods that need a "Get" prefix. (Closed)
|
Created:
4 years ago by Łukasz Anforowicz Modified:
3 years, 8 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a hardcoded list of methods that need a "Get" prefix.
BUG=582312
Committed: https://crrev.com/201a8768402bf7914118360149611268ff7f1f33
Cr-Commit-Position: refs/heads/master@{#439818}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed CR feedback from dcheng@. #Patch Set 3 : Add more methods that need "Get" prepended. #Patch Set 4 : Remove commented-out lines. #
Total comments: 4
Patch Set 5 : |const std::string&| instead of |std::string| as a parameter type. #Patch Set 6 : Introduce shouldPrefixFunctionName matcher. #Patch Set 7 : Rebasing... #
Messages
Total messages: 25 (10 generated)
lukasza@chromium.org changed reviewers: + dcheng@chromium.org, nasko@chromium.org
Daniel, can you take an overall look please? https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:352: "readyState", Nasko, can you help populate this list with more instances of the TypeName type vs typeName() method/accessor problem?
https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:343: bool IsMethodNameLikelyToConflictWithTypeName(std::string method_name) { Just call this ShouldPrefixMethodName ('likely' is a bit handwavy, since that *something* definitely conflicts if it's in this list) https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:346: method_name[0] = clang::toLowercase(method_name[0]); As discussed, I think we shouldn't need this: - if the input is NamedLikeThis, if it will conflict with something, it already does. So we just check for namesLikeThis.
https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:343: bool IsMethodNameLikelyToConflictWithTypeName(std::string method_name) { On 2016/12/19 19:26:52, dcheng wrote: > Just call this ShouldPrefixMethodName ('likely' is a bit handwavy, since that > *something* definitely conflicts if it's in this list) Done. https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:346: method_name[0] = clang::toLowercase(method_name[0]); On 2016/12/19 19:26:52, dcheng wrote: > As discussed, I think we shouldn't need this: > - if the input is NamedLikeThis, if it will conflict with something, it already > does. So we just check for namesLikeThis. Done. (OTOH, please note that this is why I also had to move where the call occurs within GetNameForDecl below)
https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:352: "readyState", On 2016/12/19 19:07:35, Łukasz Anforowicz wrote: > Nasko, can you help populate this list with more instances of the TypeName type > vs typeName() method/accessor problem? I've populated this list with the errors I've seen so far - maybe this is good enough for now (at least good enough to land this CL + retry what else is missing after rerunning the rename tool).
On 2016/12/20 00:32:05, Łukasz Anforowicz wrote: > https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... > File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): > > https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... > tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:352: "readyState", > On 2016/12/19 19:07:35, Łukasz Anforowicz wrote: > > Nasko, can you help populate this list with more instances of the TypeName > type > > vs typeName() method/accessor problem? > > I've populated this list with the errors I've seen so far - maybe this is good > enough for now (at least good enough to land this CL + retry what else is > missing after rerunning the rename tool). For the record - this is how I extracted the conflicting type names: ... pipe compile errors to all.txt grep -i 'tag to refer to type' ~/scratch/all.txt | cut -f 5- -d : | sort -u > ~/scratch/conflicts.txt cat ~/scratch/conflicts.txt | cut -f 4 -d "'" | sort > ~/scratch/conflicts2.txt And then I did a self-review of the names, commenting out (and then removing in the next patchset) ones where a "Get" prefix seemed wrong.
On 2016/12/20 00:39:58, Łukasz Anforowicz wrote: > On 2016/12/20 00:32:05, Łukasz Anforowicz wrote: > > > https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... > > File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): > > > > > https://codereview.chromium.org/2589923002/diff/1/tools/clang/rewrite_to_chro... > > tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:352: > "readyState", > > On 2016/12/19 19:07:35, Łukasz Anforowicz wrote: > > > Nasko, can you help populate this list with more instances of the TypeName > > type > > > vs typeName() method/accessor problem? > > > > I've populated this list with the errors I've seen so far - maybe this is good > > enough for now (at least good enough to land this CL + retry what else is > > missing after rerunning the rename tool). > > For the record - this is how I extracted the conflicting type names: > ... pipe compile errors to all.txt > grep -i 'tag to refer to type' ~/scratch/all.txt | cut -f 5- -d : | sort -u > > ~/scratch/conflicts.txt > cat ~/scratch/conflicts.txt | cut -f 4 -d "'" | sort > ~/scratch/conflicts2.txt > > And then I did a self-review of the names, commenting out (and then removing in > the next patchset) ones where a "Get" prefix seemed wrong. Daniel mentioned that TopLevelBlameContext looks a bit suspicious, so I am copy&pasting the errors below: ../../content/renderer/renderer_blink_platform_impl.h:296:3: error: must use 'class' tag to refer to type 'TopLevelBlameContext' in this scope TopLevelBlameContext top_level_blame_context_; ^ class ../../content/renderer/renderer_blink_platform_impl.h:199:24: note: class 'TopLevelBlameContext' is hidden by a non-type declaration of 'TopLevelBlameContext' here blink::BlameContext* TopLevelBlameContext() override;
LGTM Personally I would prefer that we just define ShouldPrefixMethodName (perhaps it should be called ShouldPrefixFunctionName? Since it's not necessarily just methods that would conflict? Not sure here...) a matcher, and compose it into the existing one that looks inside return types. https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:343: bool ShouldPrefixMethodName(std::string old_method_name) { StringRef or const std::string& https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:392: "topLevelBlameContext", This one looks surprising, as I don't see a type with this name in the Blink namespace.
On 2016/12/20 00:42:03, dcheng wrote: > Personally I would prefer that we just define ShouldPrefixMethodName (perhaps it > should be called ShouldPrefixFunctionName? Since it's not necessarily just > methods that would conflict? Not sure here...) a matcher, and compose it into > the existing one that looks inside return types. Done: - Renamed to ShouldPrefix*Function*Name - Introduced a shouldPrefixFunctionName matcher (still need to keep the raw predicate function - to be able to call this from GuessNameForUnresolvedDependentNode) https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right): https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:343: bool ShouldPrefixMethodName(std::string old_method_name) { On 2016/12/20 00:42:02, dcheng wrote: > StringRef or const std::string& Done - thanks for pointing this out (I initially had |const std::string&| here THEN changed to |std::string| to clobber the argument value to always change the first character to lower case THEN reverted the cloberring in response to code review, but forgot to change the parameter type). https://codereview.chromium.org/2589923002/diff/60001/tools/clang/rewrite_to_... tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:392: "topLevelBlameContext", On 2016/12/20 00:42:02, dcheng wrote: > This one looks surprising, as I don't see a type with this name in the Blink > namespace. Ack. Please see the error message in my other reply.
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2589923002/#ps100001 (title: "Introduce shouldPrefixFunctionName matcher.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:
While running git apply --index -p1;
error: patch failed:
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:829
error: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp: patch
does not apply
Patch: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index
97d9ebd309706e2e64d64d88f1452ea64400d3ed..13a398caf37a7545b3d64303643bf4e44a88d8ac
100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -340,6 +340,70 @@ AST_MATCHER_P(clang::QualType, hasString, std::string,
ExpectedString) {
return ExpectedString == Node.getAsString();
}
+bool ShouldPrefixFunctionName(const std::string& old_method_name) {
+ // Methods that are named similarily to a type - they should be prefixed
+ // with a "get" prefix.
+ static const char* kConflictingMethods[] = {
+ "animationWorklet",
+ "audioWorklet",
+ "binaryType",
+ "blob",
+ "channelCountMode",
+ "color",
+ "counterDirectives",
+ "document",
+ "emptyChromeClient",
+ "emptyEditorClient",
+ "emptySpellCheckerClient",
+ "entryType",
+ "error",
+ "fileUtilities",
+ "font",
+ "iconURL",
+ "inputMethodController",
+ "inputType",
+ "layout",
+ "layoutBlock",
+ "layoutSize",
+ "length",
+ "lineCap",
+ "lineJoin",
+ "matchedProperties",
+ "name",
+ "navigationType",
+ "node",
+ "outcome",
+ "pagePopup",
+ "paintWorklet",
+ "path",
+ "processingInstruction",
+ "readyState",
+ "screenInfo",
+ "scrollAnimator",
+ "settings",
+ "signalingState",
+ "state",
+ "string",
+ "text",
+ "textAlign",
+ "textBaseline",
+ "theme",
+ "timing",
+ "topLevelBlameContext",
+ "widget",
+ };
+ for (const auto& conflicting_method : kConflictingMethods) {
+ if (old_method_name == conflicting_method)
+ return true;
+ }
+
+ return false;
+}
+
+AST_MATCHER(clang::FunctionDecl, shouldPrefixFunctionName) {
+ return ShouldPrefixFunctionName(Node.getName().str());
+}
+
bool GetNameForDecl(const clang::FunctionDecl& decl,
clang::ASTContext& context,
std::string& name) {
@@ -359,7 +423,8 @@ bool GetNameForDecl(const clang::FunctionDecl& decl,
hasString(name),
// hasDeclaration matches resolved type (Foo or DerivedFoo above).
hasDeclaration(namedDecl(hasName(name))),
- hasDeclaration(cxxRecordDecl(isDerivedFrom(namedDecl(hasName(name)))))));
+ hasDeclaration(
+ cxxRecordDecl(isDerivedFrom(namedDecl(hasName(name)))))));
// |type_containing_same_name_as_function| matcher will match all of the
// return types below:
@@ -371,8 +436,9 @@ bool GetNameForDecl(const clang::FunctionDecl& decl,
hasDescendant(type_with_same_name_as_function)));
// https://crbug.com/582312: Prepend "Get" if method name conflicts with
// return type.
- auto conflict_matcher =
- functionDecl(returns(type_containing_same_name_as_function));
+ auto conflict_matcher = functionDecl(anyOf(
+ returns(type_containing_same_name_as_function),
+ shouldPrefixFunctionName()));
if (IsMatching(conflict_matcher, decl, context))
name = "Get" + name;
@@ -829,6 +895,8 @@ class UnresolvedRewriterBase : public
RewriterBase<TargetNode> {
!IsBlacklistedFunctionOrMethodName(old_name)) {
new_name = old_name;
new_name[0] = clang::toUppercase(old_name[0]);
+ if (ShouldPrefixFunctionName(old_name))
+ new_name = "Get" + new_name;
return true;
}
The CQ bit was checked by lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2589923002/#ps120001 (title: "Rebasing...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482250682062880,
"parent_rev": "3315f29c6ed4441864d1b9856880952d3b23378f", "commit_rev":
"9f4dcf83fecfb0dfab949c7c7989cd6530c884ea"}
Message was sent while issue was closed.
Description was changed from ========== Add a hardcoded list of methods that need a "Get" prefix. BUG=582312 ========== to ========== Add a hardcoded list of methods that need a "Get" prefix. BUG=582312 Review-Url: https://codereview.chromium.org/2589923002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add a hardcoded list of methods that need a "Get" prefix. BUG=582312 Review-Url: https://codereview.chromium.org/2589923002 ========== to ========== Add a hardcoded list of methods that need a "Get" prefix. BUG=582312 Committed: https://crrev.com/201a8768402bf7914118360149611268ff7f1f33 Cr-Commit-Position: refs/heads/master@{#439818} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/201a8768402bf7914118360149611268ff7f1f33 Cr-Commit-Position: refs/heads/master@{#439818}
Message was sent while issue was closed.
sashab@chromium.org changed reviewers: + sashab@chromium.org
Message was sent while issue was closed.
Sorry to revive this old CL, but why do we need a global list of methods that need a "Get" prefix? After the rename, can some of these be renamed back? Surely not all of these methods cause naming conflicts.
Message was sent while issue was closed.
On 2017/04/11 00:21:44, sashab wrote: > Sorry to revive this old CL, but why do we need a global list of methods that > need a "Get" prefix? After the rename, can some of these be renamed back? Surely > not all of these methods cause naming conflicts. The hardcoded list is because the tool didn't have global visibility into naming decisions. So things like WebFrame::Document() are actually problematic, but the tool wouldn't necessarily know that. I did suggest a way to do it so we didn't need to hardcode any lists, but it would be harder to implement and could lead to local inconsistencies when running the tool on a subset of files. Most, if not all, of these names still collide with typenames and will need Get prefixes. There's a few exception in layout code which I'm addressing. Other than layout, do you specific things in mind that we can rename back? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
