Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Push element error #163

Merged
merged 2 commits into from
Jan 27, 2024
Merged

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Dec 18, 2023

Not sure if we should make the EbmlMaster constructor throw in this case. There might be some code assuming Mandatory elements are available no matter what. This is also needed in the copy constructor of #162. But it would break the API/ABI so not for 1.x.

This only happens when pushing to a vector fails, so extremely limited memory constraints.

The API was ready for it but we didn't return an error.
It may fail on allocation of the vector.
Copy link
Contributor

@mbunkus mbunkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

Indentation — can you please use two spaces, even for the first new indentation level you introduced?

Nesting — can you please convert the code not to use all of those nesting layers? It's easy to either continue; or return or whatever earlier by inverting the tests. E.g. (untested!)

  for (EltIdx = 0; EltIdx < EBML_CTX_SIZE(MasterContext); EltIdx++) {
    if (!EBML_CTX_IDX(MasterContext,EltIdx).IsMandatory() && EBML_CTX_IDX(MasterContext,EltIdx).IsUnique())
      continue;

//  assert(EBML_CTX_IDX(MasterContext,EltIdx).Create != NULL);

    if (PushElement(EBML_SEM_CREATE(EBML_CTX_IDX(MasterContext,EltIdx))))
      continue;

    while (EltIdx-- != 0) {
      if (!EBML_CTX_IDX(MasterContext,EltIdx).IsMandatory() && EBML_CTX_IDX(MasterContext,EltIdx).IsUnique())
        continue;

      // element that have been pushed should be removed
      auto *todelete = ElementList.back();
      ElementList.pop_back();
      delete todelete;
    }

    return false;
  }

It's not handled by the caller yet.
@robUx4
Copy link
Contributor Author

robUx4 commented Jan 27, 2024

Fixed the Indentation and Nesting.

@mbunkus mbunkus merged commit 0fbab34 into Matroska-Org:master Jan 27, 2024
15 checks passed
@robUx4 robUx4 deleted the PushElement_error branch January 28, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants