Refactor output modes to use enum (#489) by valdaarhun · Pull Request #929 · rizinorg/rizin

valdaarhun · 2021-03-29T09:51:29Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the documentation and the rizin book with the relevant information (if needed)

Detailed description

Refactored output modes to use enum. int mode and char mode patterns in the code have been replaced with RzOutputMode enum type.

Refactoring is still in progress.

Test plan

...

Closing issues

closes #489

XVilka · 2021-03-29T10:51:21Z


-RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64 size, int rad, PJ *pj, bool show_full) {
-	rz_return_if_fail(!(rad == 'j' && !pj)); // rad == 'j' => pj != NULL
+RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64 size, RzOutputMode rad, PJ *pj, bool show_full) {


I suggest to rename variable too rad -> mode. And everywhere else you meet unclear name for the mode.

Ok, will do.

I have changed rad to mode wherever output modes are involved.

wargio · 2021-03-29T12:41:15Z

@@ -339,7 +339,7 @@ RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64
 			break;
 		case 0:
 		case 1:


i would remove 0 and 1.

wargio · 2021-03-29T12:41:43Z

 		case 0:
 		case 1:
-		case '*':
+		case RZ_OUTPUT_MODE_RIZIN:


no need to define this if it goes into the default switch case.

Ok, so I will remove all the three cases.

wargio · 2021-03-29T12:43:43Z

-	case 'j': print_types_json(pdb, pj, tpi_stream->types); return;
-	case 'r': print_types_format(pdb, tpi_stream->types); return;
+	case RZ_OUTPUT_MODE_JSON: print_types_json(pdb, pj, tpi_stream->types); return;
+	case RZ_OUTPUT_MODE_RIZIN: print_types_format(pdb, tpi_stream->types); return;


i'm not sure about this switch. what currently does the 'd' command?

wargio · 2021-03-29T12:44:51Z

-		rz_analysis_var_list_show(core->analysis, fcn, 'r', 0, NULL);
+		rz_analysis_var_list_show(core->analysis, fcn, 'b', RZ_OUTPUT_MODE_QUIET, NULL);
+		rz_analysis_var_list_show(core->analysis, fcn, 's', RZ_OUTPUT_MODE_QUIET, NULL);
+		rz_analysis_var_list_show(core->analysis, fcn, 'r', RZ_OUTPUT_MODE_QUIET, NULL);


i think this should be RZ_OUTPUT_MODE_STANDARD

wargio · 2021-03-29T12:45:36Z

 					}
 					break;
-				case 'r':
+				case RZ_OUTPUT_MODE_RIZIN:


i'm not sure this change is correct. that 'r' is a command, not a mode.

wargio · 2021-03-29T12:46:47Z

 		}
 	}
-	return 0;
+	return RZ_OUTPUT_MODE_QUIET;


not sure about this change either.

wargio · 2021-03-29T12:47:51Z

 		case ':':
-		case '*':
-			if (mode == '*' || (mode == ':' && addr >= map->addr && addr < map->addr_end)) {
+		case 'RZ_OUTPUT_MODE_RIZIN':


this change doesn't make sense. the command requires to be splitted.

@wargio I am sorry but I haven't understood what you mean by "command should be split". Do you mean I should have a separate case for mode == ':'? Also, I noticed that I have accidentally put RZ_OUTPUT_MODE_RIZIN in single quotes. I'll change that as well.

XVilka · 2021-03-30T05:42:34Z

-RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64 size, int rad, PJ *pj, bool show_full) {
-	rz_return_if_fail(!(rad == 'j' && !pj)); // rad == 'j' => pj != NULL
+RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64 size, RzOutputMode mode, PJ *pj, bool show_full) {
+	rz_return_if_fail(!(mode == RZ_OUTPUT_MODE_JSON && !pj)); // mode == 'j' => pj != NULL


Please also update the commentary or just remove it.

XVilka · 2021-03-30T05:44:23Z

 					s = strdup(pstr);
 				}
-				if (rad) {
+				if (mode) {


I don't think this should be if (mode), code doesn't make sense in JSON mode, for example. Lets just use some specific mode here.

@XVilka Do you mean I should change it to something more specific like if (mode == RZ_OUTPUT_MODE_STANDARD)?

XVilka · 2021-03-30T05:44:38Z

 			} break;
 			case RZ_META_TYPE_STRING:
-				if (rad) {
+				if (mode) {


Same here and below.

XVilka · 2021-03-30T05:46:29Z

 			pj_ks(pj, "cond", rz_str_get(b->cond));
 			pj_end(pj);
-		} else if (rad) {
+		} else if (mode) {


Same here, better to use a switch instead of if/else and use specific modes.

XVilka · 2021-03-30T05:48:01Z

 			break;
 		case 's':
-			flag_space_stack_list(core->flags, input[2]);
+			RzOutputMode mode;


You could extract this in a separate function

@XVilka I just wanted to confirm one thing. Should I create an RZ_API defined function in this file that I can then call in every other file as well?

XVilka · 2021-03-30T05:48:26Z

 			}
 		} else {
-			rz_flag_list(core->flags, *input, input[0] ? input + 1 : "");
+			RzOutputMode mode;


Same here, will also avoid duplication.

XVilka · 2021-03-30T05:49:04Z

 			RzAnalysisMetaItem *mi = rz_meta_get_at(core->analysis, addr, type, &size);
 			if (mi) {
-				rz_meta_print(core->analysis, mi, addr, size, input[3], NULL, false);
+				switch(input[3]){


wargio · 2021-03-30T15:48:53Z

ok, here is what i would do, because this PR is quite messy and big even to comment.

I would keep this branch and switch to dev
i would then create a new branch from dev
i would edit only 1 function at time, fix any incompatibility with the old code, rebuild and run the tests.
if the tests does not fail, then commit these changes and push them.
repeat 3 and 4 till you have converted all the functions and made a single commit for each of them.

you can open a new PR and reference that one to this.

wargio · 2021-03-30T15:50:28Z

if you prefer, we can go function by function, so we can guide you towards the resolution of the issue.

XVilka · 2021-03-31T12:10:29Z

I agree about step-by-step approach, but instead of one function would do one/two files per PR. This way export from this PR is easy - just use git diff filename or similar command.

valdaarhun · 2021-03-31T15:04:25Z

@wargio @XVilka Yeah, I think I'll do that. It'll be more organised that way.

valdaarhun · 2021-04-01T18:21:41Z

@wargio @XVilka I created a new branch and refactored one file. I built it on my local computer and didn't get any compilation error, so I decided to push it in a new PR (#950). However, I have got three failing tests and I don't understand the reason behind them.

wargio · 2021-04-02T09:27:37Z

i'll close this to discuss on the other PR.

valdaarhun added 2 commits March 24, 2021 15:07

Refactored output modes to use enum (#489)

bf46c86

Merge https://github.com/rizinorg/rizin into dev

8b34737

github-actions bot added command RzBin labels Mar 29, 2021

XVilka requested changes Mar 29, 2021

View reviewed changes

wargio reviewed Mar 29, 2021

View reviewed changes

valdaarhun added 5 commits March 29, 2021 20:20

Merge https://github.com/rizinorg/rizin into dev

b94a7c3

Refactor output modes to use enum

4adaad2

Refactor output modes to use enum

8689e99

Refactor output modes to use enum

3df56db

Rename "rad" to "mode" in context of output modes

baa3c13

XVilka requested changes Mar 30, 2021

View reviewed changes

wargio closed this Apr 2, 2021

wargio mentioned this pull request Apr 2, 2021

Refactor output modes to use enum #950

Closed

4 tasks

valdaarhun mentioned this pull request Apr 2, 2021

Refactor output modes to use enum (#489) #958

Closed

4 tasks

Uh oh!

Conversation

valdaarhun commented Mar 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valdaarhun Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valdaarhun Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wargio commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wargio commented Mar 30, 2021

Uh oh!

XVilka commented Mar 31, 2021

Uh oh!

valdaarhun commented Mar 31, 2021

Uh oh!

valdaarhun commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wargio commented Apr 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

valdaarhun Mar 29, 2021 •

edited

Loading

valdaarhun Mar 30, 2021 •

edited

Loading

wargio commented Mar 30, 2021 •

edited

Loading

valdaarhun commented Apr 1, 2021 •

edited

Loading