Closed
Bug 280537
Opened 20 years ago
Closed 17 years ago
Enhancement/Cleanup for nsNNTPProtocol class
Categories
(SeaMonkey :: MailNews: Backend, enhancement)
SeaMonkey
MailNews: Backend
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 400331
People
(Reporter: tim.ruehsen, Assigned: Bienvenu)
Details
Attachments
(1 file, 1 obsolete file)
29.80 KB,
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (compatible; Konqueror/3.3; Linux) KHTML/3.3.2 (like Gecko)
Build Identifier:
Mainly, I replaced NS_MsgSACopy/NS_MsgSACat/SendData combinations with a new
helper function called PrintfToStream.
This new function also removes the limits on some parts of the code when using
a static buffer. e.g.:
char buf[OUTPUT_BUFFER_SIZE];
PR_snprintf(buf,sizeof(buf),"GROUP %.512s"CRLF, groupname);
status = SendData(url, buf);
now looks like
status = PrintfToStream(url, "GROUP %s", groupname);
PrintfToStream() uses an 512 bytes buffer on the stack, and falls back to
memory allocation if needed. It also calls SendData() without recalculating the
data size via PL_strlen().
After all, this makes the code cleaner and reduces memory allocations.
Comments are welcome.
Reproducible: Always
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Updated•20 years ago
|
Attachment #172958 -
Flags: review?(bienvenu)
Assignee | ||
Comment 2•20 years ago
|
||
Comment on attachment 172958 [details] [diff] [review]
cvs diff
thx for looking at this code. a -uw diff would be helpful, so I don't have to
look at all the whitespace changes...
Reporter | ||
Comment 3•20 years ago
|
||
Sorry about that.
But i am not able to reconstruct my changes with 'patch' using the diff file
(says, it can't locate mailnews/news/src/nsNNTPProtocol.cpp, which is there).
My changed files where overwritten, so using patch would be my only possibility
to reconstruct them. So I can't provide you with a patch without whitespace
changes.
Please try to manage the whitespace changes. For the future I will always use
-w.
Comment 4•19 years ago
|
||
This is a new version which will work on trunk (i used the old one and applied most of the changes manually) and also fixed a little bug in it. So far it seems to work fine (post, read, cancel postings,etc.).
Attachment #172958 -
Attachment is obsolete: true
Attachment #201422 -
Flags: review?
Attachment #172958 -
Flags: review?(bienvenu)
Comment 5•19 years ago
|
||
Comment on attachment 201422 [details] [diff] [review]
Patch for trunk, -uw
This is a new version which will work on trunk (i used the old one and applied
most of the changes manually) and also fixed a little bug in it. So far it
seems to work fine (post, read, cancel postings,etc.).
Attachment #201422 -
Flags: review? → review?(bienvenu)
Comment 6•19 years ago
|
||
Ah i forgot the // XXX - FIX ME? is because Bug 313147 added a "if (!m_socketIsOpen || !m_dataBuf)" check, but m_dataBuf does no longer exist with this patch. I did not look at this further, for now i have just deleted that check for m_dataBuf.
Assignee | ||
Comment 7•19 years ago
|
||
we need the equivalent of the !m_dataBuf check, some check that we've called Cleanup on the protocol object, and gotten to where that check is w/o re-initing m_dataBuf (if you did a -uwp diff, it would be easier to give the name of the functions)
It looks like you've got tabs in your patch, e.g., +PRInt32 nsNNTPProtocol::VPrintfToStream
also, in +PRInt32 nsNNTPProtocol::VPrintfToStream, aren't you leaking datap in the case where you have to allocate it because it's too big?
Here,
+
+ if (bufSize)
+ len = bufSize;
+ else
+ len = PL_strlen(dataBuffer);
+
this can just be:
len = (bufSize) ? bufSize : PL_strlen(dataBuffer);
Why not add the logging param to +PRInt32 nsNNTPProtocol::PrintfToStream(nsIURI * aURL, const char * aFormat, ...)
instead of having two identical functions, one to pass false and one to pass true to VPrintfToStream? You can default the param to PR_TRUE so only the callers that don't want logging need to pass in the last param.
if (m_typeWanted == CANCEL_WANTED) {
- NS_MsgSACopy(&command, "HEAD ");
+ PrintfToStream(mailnewsurl, "HEAD %s%s%s"CRLF,
+ *m_path != '<' ? "<" : "",
+ m_path,
+ PL_strchr(m_path, '>') == 0 ? ">" : "");
}
else {
NS_ASSERTION(m_typeWanted == ARTICLE_WANTED, "not cancel, and not article");
- NS_MsgSACopy(&command, "ARTICLE ");
+ PrintfToStream(mailnewsurl, "ARTICLE %s%s%s"CRLF,
+ *m_path != '<' ? "<" : "",
+ m_path,
+ PL_strchr(m_path, '>') == 0 ? ">" : "");
}
Here, you're just adding more code where there was less :-( you could just do:
const char *formatString;
formatString = (m_typeWanted == CANCEL_WANTED) ? "HEAD %s%s%s"CRLF : "ARTICLE %s%s%s"CRLF;
PrintfToStream(mailnewsurl, formatString,
*m_path != '<' ? "<" : "",
m_path,
PL_strchr(m_path, '>') == 0 ? ">" : "");
Or not even use the local var formatString at all.
Comment 8•19 years ago
|
||
Comment on attachment 201422 [details] [diff] [review]
Patch for trunk, -uw
ok will probably attach a new patch
Attachment #201422 -
Flags: review?(bienvenu)
Comment 9•17 years ago
|
||
Bug 400331 is also about cleanup and more active.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•