Summary

Implement a parser for ircv3 and add unit tests to it.

Review Request #1874 — Created Oct. 1, 2022 and submitted Oct. 2, 2022, 6:23 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

PIDGIN-17585

Depends On

Reviewers

Groups

pidgin

People

Description

This change got pretty big so I didn't implement unescapping tags yet. I did
however put the unit tests in for escaped tags, but they are currently #if 0'd
out.

The unit tests are based on the msg-split test cases from https://github.com/ircdocs/parser-tests/blob/master/tests/msg-split.yaml

Testing Done

Ran the unit tests.

Commits

Summary	ID
Implement a parser for ircv3 and add unit tests to it. This change got pretty big so I didn't implement unescapping tags yet. I did however put the unit tests in for escaped tags, but they are currently #if 0'd out.	53c1b425e7d8acebe2e79bd6586cfb39fb09eb9f

Issues

Description	From	Last Updated
Wrong guard macro name.	QuLogic	Oct. 2, 2022, 3:48 a.m.
I might just be missing something about the contract around the tags_string but shouldn't this be something like if(tags_string == …	hub@cerras.dev	Oct. 1, 2022, 6:40 a.m.
If it's never NULL, then you don't need the tag_string != NULL.	QuLogic	Oct. 2, 2022, 3:49 a.m.
local_error was already checked and returned, so this can't happen.	QuLogic	Oct. 2, 2022, 3:49 a.m.
handler	QuLogic	Oct. 2, 2022, 3:49 a.m.
I'm surprised this is not triggering anything in valgrind, as g_strfreev frees both params and the strings in it. But …	QuLogic	Oct. 2, 2022, 4:24 a.m.
Inconsistent indent.	QuLogic	Oct. 2, 2022, 3:49 a.m.
Walk	QuLogic	Oct. 2, 2022, 3:49 a.m.
Remove?	QuLogic	Oct. 2, 2022, 3:49 a.m.
The spacing around the braces on .params lines is a bit inconsistent.	QuLogic	Oct. 2, 2022, 3:49 a.m.
Not a big deal, but these are swapped from the order in the test data.	QuLogic	Oct. 2, 2022, 3:49 a.m.
Similar to the other control character test, this string is probably also wrong, but the compiler isn't warning because \x035 …	QuLogic	Oct. 2, 2022, 3:50 a.m.
Ditto.	QuLogic	Oct. 2, 2022, 3:50 a.m.
This is warning about an invalid hex escape sequence with \x0305, as YAML escapes are limited to 2 digits, but …	QuLogic	Oct. 2, 2022, 3:50 a.m.
Same here.	QuLogic	Oct. 2, 2022, 3:50 a.m.
It looks like you're missing one test above this one.	QuLogic	Oct. 2, 2022, 3:51 a.m.
Missing trailing comma also.	QuLogic	Oct. 2, 2022, 3:51 a.m.
Missing trailing comma.	QuLogic	Oct. 2, 2022, 3:50 a.m.

Bugs:: PIDGIN-17585

libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)

The issue has been dropped. Show all issues

I might just be missing something about the contract around the tags_string but shouldn't this be something like if(tags_string == NULL || *tags_string == '\0')

grim Oct. 1, 2022, 6:35 a.m.

tags_string will never be NULL because g_match_info_fetch_named always returns a string and never returns NULL.

Change Summary:: Add a link to the source of the unit tests.
Description:: This change got pretty big so I didn't implement unescapping tags yet. I did
however put the unit tests in for escaped tags, but they are currently #if 0'd
out.
+
+
The unit tests are based on the msg-split test cases from https://github.com/ircdocs/parser-tests/blob/master/tests/msg-split.yaml

libpurple/protocols/ircv3/purpleircv3parser.h (Diff revision 1)
The issue has been resolved. Show all issues
```
Wrong guard macro name.
```
libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
If it's never NULL, then you don't need the tag_string != NULL.
```
libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
local_error was already checked and returned, so this can't happen.
```
libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
handler
```

libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)

The issue has been dropped. Show all issues

I'm surprised this is not triggering anything in valgrind, as g_strfreev frees both params and the strings in it.

But those strings were extracted in purple_ircv3_parser_parse_tags from middle with duplication, or is exactly trailing, which were both freed above.

So this should have been a double-free.

QuLogic Oct. 1, 2022, 9:41 a.m.

That should say 'from middle without duplication'.

grim Oct. 2, 2022, 3:43 a.m.

g_strv_builder_add dupes the value.

void                                                                            
g_strv_builder_add (GStrvBuilder *builder,                                      
                    const char   *value)                                        
{                                                                               
  g_ptr_array_add (&builder->array, g_strdup (value));                          
}

QuLogic Oct. 2, 2022, 3:53 a.m.

Ah okay, but didn't you skip using g_strsplit in order to avoid the duplication? But if the duping is fine, then this can be dropped.

grim Oct. 2, 2022, 4:03 a.m.

I did, but we'd have to resize the GStrv or something because we'd split middle, and then append trailing as another item and I don't think there are any helpers for that, so GStrvBuilder actually solves all of that.

libpurple/protocols/ircv3/tests/meson.build (Diff revision 1)
The issue has been resolved. Show all issues
```
Inconsistent indent.
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Walk
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Remove?
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
The spacing around the braces on .params lines is a bit inconsistent.
```

libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)

The issue has been resolved. Show all issues

Not a big deal, but these are swapped from the order in the test data.

grim Oct. 2, 2022, 3:39 a.m.

Their comment was with and without, but the first one has an input of ":src AWAY" which doesn't have any parameters after the command of AWAY. So, if there comment was more like without and with tests or if the input strings were swapped, then they'd be consistent.

libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)

The issue has been resolved. Show all issues

This is warning about an invalid hex escape sequence with \x0305, as YAML escapes are limited to 2 digits, but C ones aren't.

I think the only way to write it is to split the string, "coolguy!~ag@n\x02et\x03" "05w\x0fork.admin"

QuLogic Oct. 1, 2022, 10:12 a.m.

Oh, e is also a valid hex character, so there should be a split between the \x02 and the e. Same with msg below.

libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Same here.
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing trailing comma.
```

libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)

The issue has been resolved. Show all issues

Similar to the other control character test, this string is probably also wrong, but the compiler isn't warning because \x035 is okay. It would also need the splitting to be right.

libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Ditto.
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
It looks like you're missing one test above this one.
```
libpurple/protocols/ircv3/tests/test_ircv3_parser.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing trailing comma also.
```

Change Summary:

address issues

Commits:

	Summary	ID
	Implement a parser for ircv3 and add unit tests to it. This change got pretty big so I didn't implement unescapping tags yet. I did however put the unit tests in for escaped tags, but they are currently #if 0'd out.	3b049ac2de40477bb7ad243ba66d256019b83881
	Implement a parser for ircv3 and add unit tests to it. This change got pretty big so I didn't implement unescapping tags yet. I did however put the unit tests in for escaped tags, but they are currently #if 0'd out.	53c1b425e7d8acebe2e79bd6586cfb39fb09eb9f

Diff:

Revision 2 (+2326 -4)

Show changes

	libpurple/protocols/ircv3/meson.build
	libpurple/protocols/ircv3/purpleircv3core.h
	libpurple/protocols/ircv3/purpleircv3core.c
	libpurple/protocols/ircv3/purpleircv3parser.h
	libpurple/protocols/ircv3/purpleircv3parser.c
	libpurple/protocols/ircv3/tests/meson.build
	libpurple/protocols/ircv3/tests/test_ircv3_parser.c

Ship it!

```
Ship It!
```

Checked all the tests and they match the original test data.

Status:: Completed

		This change got pretty big so I didn't implement unescapping tags yet. I did
		however put the unit tests in for escaped tags, but they are currently #if 0'd
		out.
	+
	+	The unit tests are based on the msg-split test cases from https://github.com/ircdocs/parser-tests/blob/master/tests/msg-split.yaml