487874 - Piglet/pork: bugfix and enhancements

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

* fixes fencepost bug in patcher.
* adds unit test framework for patcher
* adds unit test for fencepost bug
* adds ability to redirect patch to any ostream&
* adds ostream operator<< to the unboxedloc classes

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: stuff

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

BTW, I should add that I don't like the current way piglet and its tests are built.  But this depends on the oink-stack reorg.

Comment 3

[(dormant account)]

Comment on attachment 372135 [details] [diff] [review]
stuff

Some minor comments.

> 
>-Patcher::Patcher(bool recursive):recursive(recursive) {
>-  
>+Patcher::Patcher() 
>+  : out(cout), recursive(false) {
>+}
>+
>+Patcher::Patcher(ostream& out, bool recursive)
>+  : out(out), recursive(recursive) {   
> }
> 

Should use default arguments instead of adding another constructor

>+ 
>+std::ostream& 
>+operator<<(std::ostream& os, const UnboxedLoc& ul) {
>+    return os << ul.line << ":" << ul.col;
>+}
> 
>+
>+std::ostream& 
>+operator<<(std::ostream& os, const UnboxedPairLoc& pl) {
>+    return os << pl.file <<":("<< pl.first <<", "<< pl.second <<")";
>+}

I'd put trivial things like this in the .h file.

>diff --git a/patcher.h b/patcher.h
>--- a/patcher.h
>+++ b/patcher.h
>@@ -23,16 +23,18 @@ public:
>   SourceLoc toSourceLoc(const char* file);
>   int operator<(const UnboxedLoc &rhs) const {
>     return line < rhs.line
>       || (line == rhs.line && col < rhs.col);
>   }
>   
>   unsigned int line;
>   unsigned int col;
>+
>+  friend std::ostream& operator<<(std::ostream& os, const UnboxedLoc& ul);
> };


>+
>+    friend std::ostream& operator<<(std::ostream& os, const UnboxedPairLoc& pl);
> };

These are public already, why are they friended?


> #endif // PRCHECK_PARSER
>diff --git a/test/Makefile b/test/Makefile
>new file mode 100644
>--- /dev/null
>+++ b/test/Makefile

This makefile is a bit messy, but that's understandable for now.

>diff --git a/test/patchertest-adjpatchsameline.cc b/test/patchertest-adjpatchsameline.cc
>new file mode 100644
>--- /dev/null
>+++ b/test/patchertest-adjpatchsameline.cc
>@@ -0,0 +1,3 @@
>+int main() {
>+int*x;
>+}
>diff --git a/test/patchertests.cc b/test/patchertests.cc
>new file mode 100644
>--- /dev/null
>+++ b/test/patchertests.cc
>@@ -0,0 +1,121 @@
>+#include <fstream>
>+#include <iostream>
>+#include <sstream>
>+
>+#include "expr_visitor.h"
>+#include "patcher.h"
>+#include "piglet.h"
>+
>+using namespace std;
>+
>+int failures = 0;
>+
>+class PatcherTest : public ExpressionVisitor {
>+public:
>+    PatcherTest(const string& filename)
>+        : filename_(filename), 
>+          pout_(),
>+          patcher_(pout_) {
>+    }
>+    virtual ~PatcherTest() {
>+    }
>+
>+    virtual void run() = 0;
>+
>+    void pass() {
>+        cout <<"PASS | "<< filename_ <<endl;
>+    }
>+
>+    void fail(const char* why) {
>+        cerr <<"***** FAIL | "<< filename_ <<":"<<endl<<"  "<< why <<endl;
>+        cerr <<"*****  Test log:"<< endl;
>+        cerr << log_.str() << endl;
>+        ++failures;
>+    }
>+
>+    bool samePatch(const string& as) {
>+        patcher_.flush();
>+        return as == pout_.str();
>+    }
>+
>+    ostream& log() {
>+        return log_;
>+    }
>+
>+protected:
>+    string filename_;
>+    ostringstream pout_;
>+    ostringstream log_;
>+    Patcher patcher_;
>+    PigletParser parser_;
>+};
>+
>+
>+class AdjacentPatchSameLine : public PatcherTest {
>+public:
>+    AdjacentPatchSameLine()
>+        : PatcherTest("patchertest-adjpatchsameline.cc") {
>+    }
>+    ~AdjacentPatchSameLine() {
>+    }
>+
>+    void run() {
>+        try {
>+            TranslationUnit *ast = parser_.getAST(filename_.c_str());
>+            ast->traverse(*this);
>+        }  catch (xBase &x) {
>+            fail(x.why().c_str());
>+        }
>+
>+        ostringstream patch;
>+        patch << "--- patchertest-adjpatchsameline.cc"<< endl;
>+        patch << "+++ patchertest-adjpatchsameline.cc"<< endl;
>+        patch << "@@ -2,1 +2,1 @@"<< endl;
>+        patch << "-int*x;"<< endl;
>+        patch << "+char*y;"<< endl;

this should really be a constant :)

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v2

Comments addressed.

Comment 5

[(dormant account)]

http://hg.mozilla.org/users/tglek_mozilla.com/piglet/rev/af249abd3a95