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

rdmd: support -of=... and -od=... flags as well as -of... and -od... #341

Merged
merged 2 commits into from
Mar 25, 2018

Conversation

WebDrake
Copy link
Contributor

@WebDrake WebDrake commented Mar 24, 2018

This fixes a discrepancy between rdmd and dmd in terms of the output flags they support: dmd now requests -of=... and -od=... in help output but still supports the old -of... and -od... style, while rdmd was still supporting only the older variant.

The fix itself is a bit of a hacky tweak to the dashOh callback used to parse -o flags via getopt. It seemed the simplest way to fix the issue without a more intrusive rewrite of how -o flags are parsed.

Extra test cases have been added to rdmd_test to cover this, although the way this has been done is a little bit cheeky too (it turns out the only place where -of and -od are really tested rigorously in their behaviour is in tests for the case where the output file is a library in a subdirectory). Probably at some point it would be a good idea to add some more general/rigorous test cases for the -o... flags, but in the short term the tests added in this patch should validate the changes to rdmd itself.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @WebDrake! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@WebDrake
Copy link
Contributor Author

This is just a small issue that I noticed while working on some refactoring goals. It seemed better to fix it sooner rather than later. Would this be worth adding an issue and/or changelog entry over?

@WebDrake
Copy link
Contributor Author

Updated the commit message & PR description with a bit more context for the reason behind the fix.

@CyberShadow
Copy link
Member

Thanks. I think this is more code than necessary, how about this:

    // Parse the -o option (-ofmyfile or -odmydir).
    void dashOh(string key, string value)
    {
        if (value.skipOver("f"))
        {
            // -ofmyfile passed
            value.skipOver("=");
            exe = value;
        }
        else if (value.skipOver("d"))
        {
            // -odmydir passed
            value.skipOver("=");
            if (!exe.ptr) // Don't let -od override -of
            {
                // add a trailing dir separator to clarify it's a dir
                exe = value;
                if (!exe.endsWith(dirSeparator))
                {
                    exe ~= dirSeparator;
                }
                assert(exe.endsWith(dirSeparator));
            }
        }
        else if (value == "-")
        {
            // -o- passed
            enforce(false, "Option -o- currently not supported by rdmd");
        }
        else if (value == "p")
        {
            // -op passed
            preserveOutputPaths = true;
        }
        else
        {
            enforce(false, "Unrecognized option: " ~ key ~ value);
        }
    }

The above will also fix -o-foo being incorrectly accepted.

@WebDrake
Copy link
Contributor Author

Oh, interesting thought. Yup, I can rewrite along those lines if it's convenient.

The above will also fix -o-foo being incorrectly accepted.

Ah, right. The whole value[0] check in the original is indeed problematic like this. A good case for your approach.

This fixes a discrepancy between `rdmd` and `dmd` in terms of the output
flags they support: `dmd` now requests `-of=...` and `-od=...` in help
output but still supports the old `-of...` and `-od...` style, while
`rdmd` was still supporting only the older variant.

Thanks to @CyberShadow for suggesting the `skipOver`-based solution.

Extra test cases have been added to `rdmd_test` to cover this, although
the way this has been done is a little bit cheeky (it turns out the only
place where `-of` and `-od` are really tested rigorously is in tests for
the case where the output file is a library in a subdirectory).  At some
point it would be a good idea to add some more rigorous test cases for
the `-o...` flags, but in the short term the tests added in this patch
should validate the changes to `rdmd` itself.
Copy link
Member

@CyberShadow CyberShadow left a comment

Choose a reason for hiding this comment

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

Thanks!

@WebDrake
Copy link
Contributor Author

Wait wait! I have a small tweak I want to make :-P

By checking only `value[0]`, the `dashOh` callback used to parse `-o...`
flags would give false positives (e.g. for `-o-foo` or `-opbar`).  This
patch fixes the discrepancy by requiring exact comparison to `value`.

A few test cases have been added to `rdmd_test` to cover these failures.

Thanks to @CyberShadow for spotting the bug and proposing the fix.
@WebDrake
Copy link
Contributor Author

Updated. Super picky on my part, but I wanted to tweak the commit messages slightly ;-)

@CyberShadow
Copy link
Member

Alrighty :)

@WebDrake
Copy link
Contributor Author

No changelog entry needed?

@CyberShadow
Copy link
Member

Subjective, but this seems minor enough that I wouldn't call it required. A linked Bugzilla issue would be OK too.

@WebDrake
Copy link
Contributor Author

Let's not worry about it, then, since auto-merge is already on. Thanks!

@UplinkCoder
Copy link
Member

@WebDrake Congrats!

@dlang-bot dlang-bot merged commit 9d414d2 into dlang:master Mar 25, 2018
@WebDrake WebDrake deleted the fix-of-od-flags branch March 25, 2018 19:12
@Geod24
Copy link
Member

Geod24 commented Jan 9, 2021

This PR fixed issue 17064.

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.

5 participants