Thanks Olly,<div>First I am sad to be the first negative case study, my fault to rush coding in order to deliver asap :(<br>The attachment is my revised patch which looks better.</div><div>1. The code appears well format in github, but appears mess in my vim after clone (maybe sth. wrong with tab/space indent)</div>
<div>2. So I now generate the new patch with '-w';</div><div>3. Currently the patch have not been tested and I still am working on that;</div><div>4. The modified simpleindex was intend for my own testing, which I think maybe helpful for other developers.<br>
<br><div class="gmail_quote">On Mon, Apr 16, 2012 at 10:14 PM, Olly Betts <span dir="ltr"><<a href="mailto:olly@survex.com">olly@survex.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Sat, Apr 14, 2012 at 07:02:58PM -0400, Weixian Zhou wrote:<br>
> I fixed a bug in brass_database.cc.<br>
<br>
</div>Thanks for your patches.<br>
<br>
One minor but important issue - your indentation doesn't match the<br>
existing code. Set your editor to indent by 4 spaces, with tabs<br>
displayed as 8 spaces, and indents of 8 or more spaces tab-filled.<br>
Full details are in HACKING.<br>
<br>
Also, in a patch, it's better to just remove old code rather than<br>
commenting it out. If people want to see the old code, they can look in<br>
the version control system.<br>
<br>
Looking at the flush threshold patch first, don't put your initials or<br>
name in comments you add (or at least not as a matter of course) - if<br>
people want to know who wrote a method, the version control system can<br>
tell them (e.g. git blame or svn blame). Updating copyright attribution<br>
is the big exception to that.<br>
<br>
It's better to make comments like that one in brass_inverter.h doxygen<br>
format comments so they appear in the generated documentation for<br>
internal APIs. Mostly that means use "/**" or "///" to start them,<br>
though there are various formatting rules too.<br>
<br>
There's no need to define a destructor if it's empty - you get an<br>
empty destructor by default.<br>
<br>
Inverter::get_inverter_size() is just an accessor method, so should<br>
really be const.<br>
<br>
The tracking of posting_count seems incorrect to me:<br>
<br>
* You increment it for each call to add_posting() (OK so far)<br>
* But you don't increment it at all for remove_posting(), which often<br>
also adds entries.<br>
* You only decrease it by own when a posting list is written out, not<br>
by the number of entries written.<br>
* The default threshold of 10000 is going to be too low now.<br>
<br>
It would also really be better (though much harder) to track the<br>
memory actually used, since that's what matters more to users.<br>
<br>
You don't remove the existing change_count variable, but just remove the<br>
code which updates it when documents are added or updated or removed, so<br>
it will always be zero (unless the user asks for allterms) which will<br>
cause various issues (like commit() not actually flushing changes).<br>
This causes a number of testcases to fail with your changes applied -<br>
try "make check" to see.<br>
<div class="im"><br>
> I also modified the simpleindex.cc so that it now supports batch files<br>
> indexing.<br>
<br>
</div>Interesting, but we already have omindex for indexing a directory tree,<br>
while simpleindex is intended to be *simple*. So I'm not sure we want<br>
to change it to scan directories, particularly since the code to do that<br>
depends on the OS (we have a adaptor layer for this for the MSVC build,<br>
but examples should ideally not have to drag in such code as it makes<br>
them harder to understand).<br>
<br>
A list of text files on the command line might be a reasonable<br>
alternative (or perhaps better as a separate example).<br>
<br>
Cheers,<br>
Olly<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>Weixian Zhou<br>Department of Computer Science and Engineering<br>University at Buffalo, SUNY<br><br>
</div>