Was wondering if anyone could give me some additional feedback on this small patch
I submitted for MultiMail?

https://github.com/wmcbrine/MultiMail/pull/20

That shows the feedback I got so far, clicking the "files changed" tab shows the
actual patch.

Thanks...

--
-Andy


... Got my tie caught in the fax... Suddenly I was in L.A.
-+- MultiMail/Linux v0.52

--- BBBS/Li6 v4.10 Toy-4
* Origin: Prism bbs (1:261/38)

Andrew Alt wrote to All <=-

Was wondering if anyone could give me some additional feedback on this small patch I submitted for MultiMail?

https://github.com/wmcbrine/MultiMail/pull/20

That shows the feedback I got so far, clicking the "files changed" tab shows the actual patch.

What's the patch do? I'll have look soonish.. :)

Ttyl :-),
Al

... I couldn't repair the brakes.. So I made your horn louder!
-+- MultiMail/Linux v0.52

--- BBBS/Li6 v4.10 Toy-4
* Origin: The Rusty MailBox - Penticton, BC Canada (1:153/757)

Alan Ianson wrote to Andrew Alt <=-

Andrew Alt wrote to All <=-

Was wondering if anyone could give me some additional feedback on this small patch I submitted for MultiMail?

https://github.com/wmcbrine/MultiMail/pull/20

That shows the feedback I got so far, clicking the "files changed" tab shows the actual patch.

What's the patch do? I'll have look soonish.. :)

Not much of anything really xD It's pretty trivial.. but thought it was worth doing. I narrowed the scope of newtag and put the fopen statement next to the declaration of the file descriptor, and made a slight change to the condition (which probably could just be "flag = (fd);" but the PR was closed so... guess I
won't make any more changes to it...


-+-
interfac/tagline.cc | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/interfac/tagline.cc b/interfac/tagline.cc index 3020655..a7e836d 100644
-+- a/interfac/tagline.cc
+++ b/interfac/tagline.cc
@@ -259,13 +259,11 @@ void TaglineWindow::kill()
bool TaglineWindow::ReadFile()
{
FILE *fd;
- char newtag[TAGLINE_LENGTH + 1];
- bool flag;
-
fd = fopen(tagname, "rt");
- flag = !(!fd);
+ bool flag = (fd != NULL);

if (flag) {
+ char newtag[TAGLINE_LENGTH + 1];
char *end;

curr = &head;


--
-Andy


... DalekDOS v(overflow): (I)Obey (V)ision impaired (E)xterminate
-+- MultiMail/Linux v0.52

--- BBBS/Li6 v4.10 Toy-4
* Origin: Prism bbs (1:261/38)

On 2019 Aug 09 15:01:24, you wrote to Alan Ianson:

What's the patch do? I'll have look soonish.. :)

Not much of anything really xD It's pretty trivial.. but thought it was worth doing. I narrowed the scope of newtag and put the fopen statement next to the declaration of the file descriptor, and made a slight change

to

the condition (which probably could just be "flag = (fd);" but the PR was closed so... guess I won't make any more changes to it...

the main thing i noted was no real explanation of why the changes were made... your above says a lot more than the report on the repository but it still leaves some "why?"...

)\/(ark

Once men turned their thinking over to machines in the hope that this would set
them free. But that only permitted other men with machines to enslave them.
... A high fat diet with lots of starch & a big bag of sugar helps!
---
* Origin: (1:3634/12.73)

mark lewis wrote to Andrew Alt <=-

the main thing i noted was no real explanation of why the changes were made... your above says a lot more than the report on the repository
but it still leaves some "why?"...

Just a small improvement that I noticed could be done. Not being very familiar with the codebase yet (and I'm not a C or CPP expert), I didn't see any *big* improvements I could make. Plus when contributing to a new repo and communicating
with a maintainer for the first time, I don't like to spend much time on big patches
until I know how the review process goes on smaller things.

--
-Andy


... Beep. Invalid Input. I take only cash.
-+- MultiMail/Linux v0.52

--- BBBS/Li6 v4.10 Toy-4
* Origin: Prism bbs (1:261/38)