Allon Mureinik has posted comments on this change.
Change subject: core: Add no-argument constructor check
......................................................................
Patch Set 1:
(3 comments)
Sorry for the delay, went completely under my radar.
Re: patterns/filters:
After rethinking it, I suggest we just drop the entire filtering concept and
opt for simplicity - EVERYTHING under common or compat should adhere to this
checkstyle, and this way we save ourselves the bugs of someone writing a class
called MyParms instead of MyParameters and breaking GWT.
If we opt to leave the filtering, I accept that FilteringAPI is not the right
way to go (as commented by Vojtec), but we should definitely fail the build if
given an illegal regex, as agreed inline.
Regardless, I have a couple mote implementation comments, inline, but nothing
there seems to major.
....................................................
File
build-tools-root/ovirt-checkstyle-extension/src/main/java/checks/NoArgConstructorCheck.java
Line 44: }
Line 45:
Line 46: @Override
Line 47: public void visitToken(DetailAST classDef) {
Line 48: if (!isClass(classDef) || !shouldCheckClass(classDef)) {
IIUC, the isClass check is redundant since getDefaultTokens() already ensures
you're only visiting CLASS_DEF items.
Line 49: return;
Line 50: }
Line 51:
Line 52: final DetailAST objBlock =
classDef.findFirstToken(TokenTypes.OBJBLOCK);
Line 58: if (child.getType() == TokenTypes.CTOR_DEF) {
Line 59: hasExplicitCtor = true;
Line 60: DetailAST ctorParams =
child.findFirstToken(TokenTypes.PARAMETERS);
Line 61: if (ctorParams.getChildCount() == 0) {
Line 62: hasNoArgCtor = true;
I'd just return here. But I guess it's a matter of personal style.
Line 63: break;
Line 64: }
Line 65: }
Line 66: child = child.getNextSibling();
Line 77: return classDef.findFirstToken(TokenTypes.LITERAL_CLASS) !=
null;
Line 78: }
Line 79:
Line 80: boolean shouldCheckClass(DetailAST classDef) {
Line 81: return filterPattern != null ?
filterPattern.matcher(getFullyQualifiedClassName(classDef)).matches() : true;
filterPattern should never be null (after you change the build to fail if the
pattern does not compile), so the ternary expression here can be ommited.
Line 82: }
Line 83:
Line 84: String getFullyQualifiedClassName(DetailAST classDef) {
Line 85: String prefix = resolvePackagePrefix(classDef);
--
To view, visit http://gerrit.ovirt.org/20274
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4f9a00d6289374b433ed4419552420a3337da50
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
