Skip to content
Snippets Groups Projects

Further refactoring towards steamrt/tasks#595

Merged Simon McVittie requested to merge wip/task595-refactor into main
2 unresolved threads
  • pv-adverb: Factor out parsing of --ld-preload, --ld-audit arguments

    This allows it to have test coverage.

  • pv-wrap: Use PvPreloadVariableIndex to represent LD_PRELOAD vs. LD_AUDIT

    Convert to a concrete pv-adverb option name at the last possible moment.

    This will help us to centralize the code to map to/from pv-adverb command-line options.

  • pv-wrap: Use an index into pv_multiarch_tuples to represent architecture

    This will help us to centralize the code that maps to/from pv-adverb command-line options.

  • pv-wrap: Build adverb arguments from PvAdverbPreloadModule structs

    This means all the parsing and serialization is in one place, and we can round-trip between the struct representation and the command-line option representation.

  • pv-adverb: Factor out processing of each PvAdverbPreloadModule

    No functional change. Best reviewed with git show --ignore-space-change.


As usual with refactoring, best reviewed a commit at a time.

/cc @denittis @refi64

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
198 *
199 * Returns: %TRUE if the argument was parsed successfully
200 */
201 gboolean
202 pv_adverb_preload_module_parse_adverb_cli (PvAdverbPreloadModule *self,
203 const char *option,
204 PvPreloadVariableIndex which,
205 const char *value,
206 GError **error)
207 {
208 g_auto(GStrv) parts = NULL;
209 const char *architecture = NULL;
210 gsize abi_index = PV_UNSPECIFIED_ABI;
211 gsize abi;
212
213 g_return_val_if_fail (self->name == NULL, FALSE);
  • Suggested change
    222 g_return_val_if_fail (self->name == NULL, FALSE);
    222 g_return_val_if_fail (self != NULL, FALSE);
    223 g_return_val_if_fail (self->name == NULL, FALSE);

    Nit: to avoid segmentation fault if we pass a NULL self

  • This is always a trade-off: anything we assert about in places like this is already meant to be a "can't happen" situation, and there's such a thing as having too many preconditions (making it harder to see the important ones).

    In public API of a library I'd definitely do as you suggest, but in this internal function I think my inclination is to not. If self can somehow be null, we'll find out very quickly anyway, when it segfaults.

  • Please register or sign in to reply
  • Ludovico de Nittis
  • 261 261 self->abi_index = abi_index;
    262 262 return TRUE;
    263 263 }
    264
    265 gchar *
    266 pv_adverb_preload_module_to_adverb_cli (PvAdverbPreloadModule *self)
    267 {
    268 g_autoptr(GString) buf = NULL;
    269 const PvPreloadVariable *variable;
    270
    271 g_return_val_if_fail (self->index_in_preload_variables < G_N_ELEMENTS (pv_preload_variables), NULL);
    • Suggested change
      280 g_return_val_if_fail (self->index_in_preload_variables < G_N_ELEMENTS (pv_preload_variables), NULL);
      280 g_return_val_if_fail (self != NULL, NULL);
      281 g_return_val_if_fail (self->index_in_preload_variables < G_N_ELEMENTS (pv_preload_variables), NULL);

      Same here?

    • Same reasoning here.

    • Please register or sign in to reply
  • Just those two very minor comments. Can definitely be merged as-is.

  • Ludovico de Nittis approved this merge request

    approved this merge request

  • This allows it to have test coverage.

    Definitely out of scope here, but it would be great to add the coverage report and let Gitlab parse it. I did it in a couple of projects already and it is very cool.

  • Simon McVittie mentioned in commit 904d6432

    mentioned in commit 904d6432

  • Please register or sign in to reply
    Loading