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

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

librz/analysis/meta.c

@@ -237,8 +237,8 @@ RZ_API const char *rz_meta_type_to_string(int type) {
 	return "# unknown meta # ";
 }

-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

librz/analysis/meta.c

@@ -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

librz/analysis/meta.c

@@ -339,7 +339,7 @@ RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64
 			break;
 		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

librz/bin/pdb/pdb.c

-	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

librz/core/canalysis.c

-		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

librz/core/canalysis.c

@@ -4464,11 +4464,11 @@ RZ_API int rz_core_analysis_search(RzCore *core, ut64 from, ut64 to, ut64 ref, i
 						op.size = 1;
 					}
 					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

librz/core/cmd_api.c

@@ -260,7 +260,7 @@ static RzOutputMode suffix2mode(const char *suffix) {
 			return argv_modes[i].mode;
 		}
 	}
-	return 0;
+	return RZ_OUTPUT_MODE_QUIET;


not sure about this change either.

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

librz/core/cmd_debug.c

@@ -1300,8 +1300,8 @@ static void cmd_debug_modules(RzCore *core, int mode) { // "dmm"
 			pj_end(pj);
 		} break;
 		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

librz/analysis/meta.c

-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

librz/analysis/meta.c

@@ -348,7 +348,7 @@ RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64
 				if (!s) {
 					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

librz/analysis/meta.c

@@ -370,7 +370,7 @@ RZ_API void rz_meta_print(RzAnalysis *a, RzAnalysisMetaItem *d, ut64 start, ut64
 				free(s);
 			} break;
 			case RZ_META_TYPE_STRING:
-				if (rad) {
+				if (mode) {


Same here and below.

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

librz/bp/bp.c

@@ -306,7 +306,7 @@ RZ_API int rz_bp_list(RzBreakpoint *bp, int rad) {
 			pj_ks(pj, "data", rz_str_get(b->data));
 			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

librz/core/cmd_flag.c

@@ -1216,7 +1216,35 @@ RZ_IPI int rz_cmd_flag(void *data, const char *input) {
 			}
 			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

librz/core/cmd_flag.c

@@ -1456,7 +1484,35 @@ RZ_IPI int rz_cmd_flag(void *data, const char *input) {
 				free(s);
 			}
 		} 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

librz/core/cmd_meta.c

 		if (input[2] == '.') { // "Cs.."
 			ut64 size;
 			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

XVilka requested a review from ret2libc March 30, 2021 05:51

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

valdaarhun commented Mar 29, 2021

XVilka Mar 29, 2021

valdaarhun Mar 29, 2021

valdaarhun Mar 29, 2021

wargio Mar 29, 2021

wargio Mar 29, 2021

valdaarhun Mar 29, 2021 •

edited

Loading

wargio Mar 29, 2021

wargio Mar 29, 2021

wargio Mar 29, 2021

wargio Mar 29, 2021

wargio Mar 29, 2021

valdaarhun Mar 30, 2021 •

edited

Loading

XVilka Mar 30, 2021

XVilka Mar 30, 2021

valdaarhun Mar 30, 2021

XVilka Mar 30, 2021

XVilka Mar 30, 2021

XVilka Mar 30, 2021

valdaarhun Mar 30, 2021

XVilka Mar 30, 2021

XVilka Mar 30, 2021

wargio commented Mar 30, 2021 •

edited

Loading

wargio commented Mar 30, 2021

XVilka commented Mar 31, 2021

valdaarhun commented Mar 31, 2021

valdaarhun commented Apr 1, 2021 •

edited

Loading

wargio commented Apr 2, 2021

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

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

Conversation

valdaarhun commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valdaarhun Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valdaarhun Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wargio commented Mar 30, 2021 • edited Loading

wargio commented Mar 30, 2021

XVilka commented Mar 31, 2021

valdaarhun commented Mar 31, 2021

valdaarhun commented Apr 1, 2021 • edited Loading

wargio commented Apr 2, 2021

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