Syntax Commentary 2
> + virtual void postvisitTopForm(TopForm *obj) { notice(obj->loc); }
> + virtual void postvisitFunction(Function *obj) { notice(obj->getLoc()); }
> + virtual void postvisitPQName(PQName *obj) { notice(obj->loc); }
> + virtual void postvisitTypeSpecifier(TypeSpecifier *obj) { notice(obj->loc); }
> + virtual void postvisitEnumerator(Enumerator *obj) { notice(obj->loc); }
> + virtual void postvisitMember(Member *obj) { notice(obj->loc); }
> + virtual void postvisitDeclarator(Declarator *obj) { notice(obj->getLoc()); }
> + virtual void postvisitIDeclarator(IDeclarator *obj) { notice(obj->loc); }
> + virtual void postvisitStatement(Statement *obj) { notice(obj->loc); }
> + virtual void postvisitInitializer(Initializer *obj) { notice(obj->loc); }
> + virtual void postvisitTemplateParameter(TemplateParameter *obj) { notice(obj->loc); }
> + virtual void postvisitDesignator(Designator *obj) { notice(obj->loc); }
> + virtual void postvisitAttribute(Attribute *obj) { notice(obj->loc); }
I never said this before so there is no way you could know, but I
think lines of code that are semantically parallel and adjacent are
much clearer if the parallelism is emphasized by ordering the
semantics vertically into columns as follows.
virtual void notice(SourceLoc loc);
virtual void postvisitTopForm(TopForm *obj) { notice(obj->loc); }
virtual void postvisitFunction(Function *obj) { notice(obj->getLoc());}
virtual void postvisitPQName(PQName *obj) { notice(obj->loc); }
virtual void postvisitTypeSpecifier(TypeSpecifier *obj)
{ notice(obj->loc); }
virtual void postvisitEnumerator(Enumerator *obj) { notice(obj->loc); }
virtual void postvisitMember(Member *obj) { notice(obj->loc); }
virtual void postvisitDeclarator(Declarator *obj) { notice(obj->getLoc());}
virtual void postvisitIDeclarator(IDeclarator *obj) { notice(obj->loc); }
virtual void postvisitStatement(Statement *obj) { notice(obj->loc); }
virtual void postvisitInitializer(Initializer *obj) { notice(obj->loc); }
virtual void postvisitTemplateParameter(TemplateParameter *obj)
{ notice(obj->loc); }
virtual void postvisitDesignator(Designator *obj) { notice(obj->loc); }
virtual void postvisitAttribute(Attribute *obj) { notice(obj->loc); }
Notice how the differences jump out at you now: it is really obvious
that you are calling getLoc() twice, whereas the others have a loc
field.
Note that Scott refuses to use this idea.
> + // During the first traversal find out the base class
If you want to use the classic start-with-a-capital style of writing
sentences, do end them with a period.
> +MinMaxLineVisitor::MinMaxLineVisitor() :
> + minLine(INT_MAX), maxLine(0), minCol(INT_MAX) {
> +}
I have no idea why, but it seems much clearer to me like this:
MinMaxLineVisitor::MinMaxLineVisitor()
: minLine(INT_MAX), maxLine(0), minCol(INT_MAX)
{}
Or this:
MinMaxLineVisitor::MinMaxLineVisitor()
: minLine(INT_MAX)
, maxLine(0)
, minCol(INT_MAX)
{}
> +static void printHunkHeaderAndDeletedLines(const MinMaxLineVisitor &mmlv, int added_lines, StringRef file) {
Line too long! Please don't exceed whatever the standard is, I think 72 columns.
> + if(mmlv.minLine <= line && mmlv.maxLine >= line) {
Let's go with spaces after the 'if':
if (mmlv.minLine <= line && mmlv.maxLine >= line) {
> +/** If m1 is a pure
> + * virtual declaration for m2 then return true. */
> +bool sameMember(Member *m1, Member *m2) {
I am curious; what is this syntax?: /** */
Is it some kind of Literate Programming/Javadoc thing? Do you have a
tool that processes this? If so, it might be a cool way to document
the code. However, in the absence of such a thing, again, please use
'//' end of line comments for multi-line comments and avoid '/* */'
comments as much as possible, preferredly never using them at all.
// If m1 is a pure virtual declaration for m2 then return true.
bool sameMember(Member *m1, Member *m2) {
Also again, in emacs if you type 'M-q' inside a comment, emacs will
format it for you just as if it were formatting a paragraph in
fundamental mode or text mode.
> + if(!(m1->ifMR_decl() && m2->ifMR_decl()))
> + return false;
Arg: curlies or put on to the same line! Also, make the 'not' more
prominent.
Ok:
if (! (m1->ifMR_decl() && m2->ifMR_decl()) ) return false;
Ok:
if (! (m1->ifMR_decl() && m2->ifMR_decl()) ) {
return false;
}
I think this is easiest to read:
bool const bothDecls = m1->ifMR_decl() && m2->ifMR_decl();
if (!bothDecls) return false;
> + for(;!it1.isDone();
> + it1.adv(), it2.adv()) {
If you have an empty clause in a 'for' make that more obvious with at
least some whitespace.
for(; !it1.isDone();
it1.adv(), it2.adv()) {
Or a comment:
for(/**/; !it1.isDone();
it1.adv(), it2.adv()) {
+RewriteVisitor::RewriteVisitor(StringRef implementationClass):
+ implementationClass(implementationClass), implementationFile(NULL), interfaceClass(NULL), interfaceFile(NULL), interfaceMembers(NULL)
+{
+}
Arg! Line too long! This is much easier to read:
RewriteVisitor::RewriteVisitor(StringRef implementationClass)
: implementationClass(implementationClass)
, implementationFile (NULL)
, interfaceClass (NULL)
, interfaceFile (NULL)
, interfaceMembers (NULL)
{}
> + const string prefix = str.size() > n && str[n] == '+' ? "" : "+ ";
Write the type as follows: string const prefix
> + if(!obj->isTS_classSpec())
> + return true;
Again, use curlies or put the 'then' clause onto the same line.
+ if(c->bases->first()) {
+ interfaceClass = c->bases->first()->name->getName();
+ } else {
+ cerr << implementationClass << " does not derive from anything" << endl;
+ exit(1);
+ }
Throw an exception here instead of using exit. The easy way to do
that is to use userFatalError() from oink/oink_util.h:
} else {
userFatalError(c->loc, "class %s does not derive from anything", implementationClass.c_str());
}
> + cout << "? interface class is " << interfaceClass << " in " << interfaceFile << ":"<getLine(obj->loc)<< endl;
Line too long.
> + for(ASTListMutator implMemberIt(c->members->list); !implMemberIt.isDone(); implMemberIt.adv()) {
Line too long; write it like this:
for(ASTListMutator implMemberIt(c->members->list);
!implMemberIt.isDone();
implMemberIt.adv()) {
> + for(ASTListIterNC implMemberIt(c->members->list);!implMemberIt.isDone();implMemberIt.adv()) {
Put spaces after semi-colons; if you can't fit, then use a newline as
above; do all 3 clauses the same way.
> + if(!implMemberIt.data()->ifMR_accessC() && !(implMemberIt.data()->ifMR_decl() && (!implMemberIt.data()->ifMR_decl()->d->decllist->isNotEmpty() ||
> + !implMemberIt.data()->ifMR_decl()->d->decllist->first()->var->hasFlag(DF_IMPLICIT)))) {
> + continue;
> + }
WTF!! This is impossible to read; what are you thinking! Way too
many layers of 'not'; too many layers of nested of parens; too much
redundancy -- CODE *IS* WRITING!
1) Set implMemberIt.data() to something at the top of the loop.
Member *m0 = implMemberIt.data();
// this is better, but still hard to read due to negatives and nesting
if (!m0->ifMR_accessC() &&
!(m0->ifMR_decl() &&
(!m0->ifMR_decl()->d->decllist->isNotEmpty() ||
!m0->ifMR_decl()->d->decllist->first()->var->hasFlag(DF_IMPLICIT))
)
)
2) Use intermediate booleans with self-documenting names.
bool const isBoink = // long horrible computation
bool const isBlort = // long horrible computation
if (!isBoink && !isBlort) continue;
3) Avoid double negatives where possible: !foo->isNotEmpty()
4) FYI: In Scott's roll-your-own RTTI idiom, for each subclass XXX
there are isXXX(), ifXXX(), and asXXX() methods. isXXX() returns
bool, ifXXX() returns the downcast object or NULL (just like
dynamic_cast()), and asXXX() downcasts and throws an exception
if it cannot (such as dynamic_cast()).
I suppose its possible that I made a mistake, not knowing exactly what
you are trying to do, but if I did that's even the more evidence that
it was too hard to read.
bool const isAccessC = m0->isMR_accessC();
bool emptyDecl = false;
bool explicitDecl = false;
if (m0->isMR_decl()) {
FakeList *dl = m0->asMR_decl()->d->decllist;
emptyDecl = dl->isEmpty();
explicitDecl = !emptyDecl && !dl->first()->var->hasFlag(DF_IMPLICIT);
}
if (!isAccessC && !(emptyDecl || explicitDecl)) continue;
The lesson is: 5) take things apart bottom-up and 6) keep re-reading
it until each step is clear.
> +#include "squash_cmd.h" // this module
> +#include "oink_cmd_util.h" // HANDLE_FLAG
Comments not lined up again.
> + // please prefix the names of flags with arguments with '-d-'
> + if (streq(arg, "-sq-squash")) {
Fix the comment along with the code: you have '-sq-' in the string but
'-d-' in the comment. I always use find-replace for such work and
*never* trust my eyes to find it.