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

Remove the original converter of LLVM IR into BIR from llvmir2hll #509

Merged
merged 2 commits into from
Feb 28, 2019

Conversation

s3rvac
Copy link
Member

@s3rvac s3rvac commented Feb 26, 2019

Now that retdec-llvmir2hll is using the new converter of LLVM IR into BIR, we can remove the original (old) one.

Reasons:

  • Nobody will probably ever use it.
  • We will not have to maintain two converters.
  • Fewer .cpp files to compile.

Notes:

  • We have to preserve labels_handler.cpp from the original converter as this module is also used by the new converter.
  • The retdec-decompiler.py script had to be also updated (to prevent users from specifying 'orig' via --backend-llvmir2bir-converter.

Closes #508.

@PeterMatula
Copy link
Collaborator

Does it make sense to still call the converter new if it is the only one left? I.e. what about changing source file names, comments, etc.
The question is, if the architecture allows to have more than one converter, and we remove new from its name, how will we ID it? In case there is ever another converter.

@s3rvac
Copy link
Member Author

s3rvac commented Feb 27, 2019

Good point. I see two options:

  1. Rename the converter to e.g. current and keep the possibility of having more converters.
  2. Remove support for having more converters and keep just a single one (without a name).

I would go with number 2 as I do not believe we will have another converter in the future. Also, having multiple converters would be unnecessarily hard to maintain (we would have to write regression tests for both of them).

What is your take on this?

@s3rvac
Copy link
Member Author

s3rvac commented Feb 27, 2019

Based on a discussion, we have decided to go with option number 2. I will adjust the PR to reflect that.

@s3rvac s3rvac assigned s3rvac and unassigned PeterMatula Feb 28, 2019
Now that retdec-llvmir2hll is using the new converter of LLVM IR into BIR, we
can remove the original (old) one.

Reasons:
* Nobody will probably ever use it.
* We will not have to maintain two converters.
* Fewer `.cpp` files to compile.

Notes:
* We have to preserve labels_handler.cpp from the original converter as this
  module is also used by the new converter.
* The retdec-decompiler.py script had to be also updated (to prevent users from
  specifying 'orig' via --backend-llvmir2bir-converter.
…onverters.

We do not believe that we will have another converter in the future. Also,
having multiple converters would be unnecessarily hard to maintain (we would
have to write regression tests for both of them).
@s3rvac s3rvac force-pushed the removal-of-original-converter-in-llvmir2hll branch from b12e832 to 9759427 Compare February 28, 2019 08:42
@s3rvac
Copy link
Member Author

s3rvac commented Feb 28, 2019

Done in commit 9759427.

@s3rvac s3rvac merged commit 5de7439 into master Feb 28, 2019
@s3rvac s3rvac deleted the removal-of-original-converter-in-llvmir2hll branch February 28, 2019 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants