On 9/17/07, Duncan Sands <baldrick at free.fr> wrote:
> Hi,
>
> > Rewrite of andersen's to be about 100x faster, cleaner, and begin to support field sensitivity
>
> some nitpicking comments:
>
> > +// without any issues. To wit, an indirect call Y(a,b) is equivalence to
>
> equivalence -> equivalent
Fixed
>
> > STATISTIC(NumIters , "Number of iterations to reach convergence");
> > STATISTIC(NumConstraints , "Number of constraints");
> > STATISTIC(NumNodes , "Number of nodes");
> > -STATISTIC(NumEscapingFunctions, "Number of internal functions that escape");
> > -STATISTIC(NumIndirectCallees , "Number of indirect callees found");
> > +STATISTIC(NumUnified , "Number of variables unified");
>
> there are now a lot of pointless spaces here. How about aligning commas on the
> end of NumConstraints?
Sure
>
> > + /// Constraint - Objects of this structure are used to represent the various
> > + /// constraints identified by the algorithm. The constraints are 'copy',
> > + /// for statements like "A = B", 'load' for statements like "A = *B",
> > + /// 'store' for statements like "*A = B", and AddressOf for statements like
> > + /// A = alloca; The Offset is applied as *(A + K) = B for stores,
> > + /// A = *(B + K) for loads, and A = B + K for copies. It is
> > + /// illegal on addressof constraints (Because it is statically
>
> Because -> because
Fixed
>
> Also, in this and other comments some lines are rather short, and I can't always see why
> you break them early.
I just do whatever emacs fill-paragraph does. I'll rerun it on the comments :)
>
> > + struct Node {
> > + Value *Val;
> > + SparseBitVector<> *Edges;
>
> strange indentation of Val.
Fixed
>
> > + // compression. NodeRep gives the index into GraphNodes
> > + // representative for this one.
>
> Probably "the GraphNodes representative"
Fixed
>
> > + unsigned NodeRep; public:
>
> shouldn't "public:" be on the next line?
>
Fixed
> > +
> > + Node() : Val(0), Edges(0), PointsTo(0), OldPointsTo(0), Changed(false),
> > + NodeRep(SelfRep) {
> > + }
>
> Closing } could follow the opening { on the previous line.
Fixed
>
> > + // Map from graph node to maximum K value that is allowed (For functions,
>
> For -> for
Fixed
>
> > + // Stack for Tarjans
>
> Tarjan's
Fixed
> + // This may look a bit ugly, but what it does is allow us to process
> > + // both store and load constraints with the same function.
>
> with the same function -> within the same function (?)
No. It really should be "with the same code"
> > + // Need to increment the member by K since that is where we are
> > + // supposed to copy to/from
>
> Missing full stop.
Fixed
>
> > + // Node that in positive weight cycles, which occur in address taking
>
> Node that -> Note that
Fixed.
Thanks!
I have something that spell checks comments, but not something that
grammar checks them, so these things are always helpful :)