Fix issue 18322: __FILE_FULL_PATH__ doesn't work as a template parameter #7798

marler8997 · 2018-01-28T12:11:12Z

No description provided.

dlang-bot · 2018-01-28T12:11:15Z

Thanks for your pull request, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18322	void fun(string file=FILE_FULL_PATH)() returns relative path (pointing to nowhere)

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

wilzbach · 2018-01-28T13:55:54Z

src/dmd/expression.d

+                s = FileName.combine(sc._module.srcfilePath, sc._module.srcfile.name.toChars());
+        }
+        else
+           s = loc.isValid() ? loc.filename : sc._module.ident.toChars();


NIt: DStyle says the bracing style should be consistent for the block.

wilzbach · 2018-01-28T13:57:28Z

src/dmd/root/filename.d

@@ -98,6 +99,19 @@ nothrow:
        }
    }

+    /********************************


FYI: We recently clarified the recommended best practices for D documentation: https://dlang.org/dstyle.html#phobos_documentation, but unfortunately DMD has still a lot of historical baggage, so don't mind about this. It's not a requirement for DMD at all.

not a problem, I've modified it to use the recommended style.

wilzbach · 2018-01-28T14:00:03Z

test/runnable/imports/test18322import.d

+        assert(fileFullPath[0] == '/');
+        assert(templateFileFullPath[0] == '/');
+        enum lastPart = "test/runnable/test18322.d";
+    }


Hmm, how about just checking for the end of the file name?

I'm not sure whether LDC or GDC use a similar setup to run the test suite

It would allow convenient execution of the test with rdmd or dmd -i

Also your test at https://github.com/dlang/dmd/blob/master/test/compilable/line.d doesn't uses the test directory.

ok, I removed the test part of the path. I think having runnable/<test-name> is probably ok.

wilzbach · 2018-01-28T14:04:21Z

src/dmd/expression.d

-            s = FileName.combine(sc._module.srcfilePath, s);
+        {
+            if (loc.isValid())
+                s = FileName.toAbsolute(loc.filename);


I'm not sure that getcwd will work in the general case. With -i and -I the file could be anywhere. As a start, we might want to add a test for more complicated cases?

Ok, I've refactored the change a bit. I've added a couple new fields to Loc. One called path that can hold a path to filename. Note that the path can be null if filename is absolute or relative to CWD.

I also added a field called filenameType which tracks whether filename came from an actual file or was generated from a mixin or #line. This makes the code simpler in general for __FILE_FULL_PATH__ because now it only generates an absolute path if the name represents a real file.

Ok, we've gone back to the original implementation. @ibuclaw has assured me that filename will always either be absolute or relative to CWD. He says this MUST be the case otherwise it will break the debugger. Also, since I couldn't find a counter-example it sounds like a reasonable thing to assume for now. If we ever do find a counter-example we can address it later (with a test!).

ibuclaw · 2018-01-29T20:18:12Z

Is the enum still necessary after simplifying the logic?

marler8997 · 2018-01-29T20:24:49Z

The requirement is to be able to tell when Loc.filename is an actual file, or it's been overriden via #line or mixin code. Maybe there's another solution, but I thought the additional field in Loc was nice to have anyway.

marler8997 · 2018-01-29T21:17:02Z

Ok, I went with a different solution. I've modified __FILE_FULL_PATH__ to always return an absolute path whether or not Loc.filename is a mixin or a #line. I don't think this change will be a big deal since #line and __FILE_FULL_PATH__ don't really make sense together no matter how you implement it.

JinShil · 2018-01-31T06:54:37Z

I restarted Jenkins

marler8997 · 2018-01-31T17:47:16Z

It appears there are no objections, merge?

JinShil · 2018-02-01T17:08:26Z

src/dmd/expression.d

@@ -1823,7 +1823,7 @@ extern (C++) abstract class Expression : RootObject
    }

    /****************************************
-     * Resolve __FILE__, __LINE__, __MODULE__, __FUNCTION__, __PRETTY_FUNCTION__ to loc.
+     * Resolve __FILE__, __LINE__, __MODULE__, __FUNCTION__, __PRETTY_FUNCTION__, __FILE_FULL_PATH__ to loc.


I'd prefer this if it was named __ABSOLUTE_FILE_PATH__

timotheecour · 2018-02-01T17:09:50Z

thats orthogonal to this bug fixing pr though

timotheecour · 2018-02-04T05:47:13Z

ping?

marler8997 · 2018-02-05T17:09:52Z

can I get a merge up in here?

wilzbach

OK, I'm putting this on the 72h merge if no further objection are raised queue.

wilzbach · 2018-01-31T11:56:13Z

src/dmd/parse.d

-                } else {
+                if (loc.isValid())
+                    s = FileName.toAbsolute(loc.filename);
+                else
                    s = FileName.combine(mod.srcfilePath, srcfile);


This was never covered :/

I wasn't actually able to create a test that exercised this code path. I added some debugging to see if any files ever had loc.invalid() as false, and it was only with object.d/__entrypoint.d and also a couple phobos modules like std.array but the code was at line 0. I'm guessing it was parsing code that might have been generated internally (not from a file). So for now I've just added an assert that prints a nice error message if this code path ever does get taken.

wilzbach · 2018-01-31T11:57:10Z

src/dmd/root/filename.d

+    */
+    extern (C++) static const(char)* toAbsolute(const(char)* name, const(char)* base = null)
+    {
+        return absolute(name) ? name : combine( base ? base : getcwd(null, 0), name);


FYI: I think DStyle would be combine(base ? base

wilzbach · 2018-02-06T22:41:27Z

test/runnable/test18322.d

+    auto filename = "runnable" ~ sep ~ "test18322.d";
+
+    fun(filename);
+    mixin(`fun(filename ~ "-mixin-16");`);


That was unexpected for me. I didn't expected that __FILE_FULL_PATH__ changes for mixins to sth. like test18322.d-mixin-16, but it's really the current behavior - https://run.dlang.io/is/F4ixWD

Hmm, yeah this is an interesting case. I would expect __FILE__ to match __FILE_FULL_PATH__ though, and __FILE__ goes to the mixin. I think if we want to change it we should probably change both...

marler8997 · 2018-02-07T23:25:36Z

Note: I made an additional change. I realized that the new field I added a while back Module.srcfilePath was actually unnecessary. Given @ibuclaw's new information that relative filenames will always be relative to CWD (and I verified this in all the code paths that I saw), this field was rendered useless. So I've removed it in this PR as well. Note that this additional change affected about a dozen lines of code.

ibuclaw · 2018-02-10T13:37:45Z

src/dmd/dmodule.d

@@ -59,9 +59,8 @@ version(Windows) {
 * Returns:
 *      NULL if it's not different from filename.
 */
-private const(char)* lookForSourceFile(const(char)** path, const(char)* filename)
+private const(char)* lookForSourceFile(const(char)* filename)


You removed a parameter, but didn't update the documentation.

nice catch...fixed

ibuclaw · 2018-02-10T13:44:47Z

src/dmd/globals.d

@@ -378,6 +378,8 @@ alias d_uns64 = uint64_t;
 // file location
 struct Loc
 {
+    // Note: it is assumed that filename is always either absolute or
+    //       relative to CWD
    const(char)* filename;


Maybe you should make this a comment against the field, such as:

const(char)* filename; // either absolute or relative to cwd

Make it a definitive comment, rather than a presumptuous one.

ibuclaw · 2018-02-10T13:47:07Z

Fine after nits.

dlang-bot added the Severity:Bug Fix label Jan 28, 2018

marler8997 force-pushed the bug18322 branch 6 times, most recently from 8d61658 to 052578f Compare January 28, 2018 13:32

wilzbach reviewed Jan 28, 2018

View reviewed changes

marler8997 force-pushed the bug18322 branch 3 times, most recently from 6178d25 to f6b6ac5 Compare January 28, 2018 18:30

marler8997 requested review from ibuclaw and dnadlinger as code owners January 28, 2018 18:30

marler8997 force-pushed the bug18322 branch 3 times, most recently from f38d0f2 to 7bfaed2 Compare January 29, 2018 17:20

dlang deleted a comment from marler8997 Jan 29, 2018

marler8997 force-pushed the bug18322 branch from 7bfaed2 to 4066c08 Compare January 29, 2018 20:07

marler8997 force-pushed the bug18322 branch from 4066c08 to f8dd437 Compare January 29, 2018 21:15

marler8997 force-pushed the bug18322 branch 5 times, most recently from 2115e31 to 502414f Compare January 29, 2018 22:01

timotheecour mentioned this pull request Jan 30, 2018

[WIP] simpler fix for issue 18322 ___FILE_FULL_PATH___ #7808

Closed

2 tasks

timotheecour added a commit to timotheecour/dmd that referenced this pull request Jan 30, 2018

added test cases from @marler8997 in PR dlang#7798

2080dc3

marler8997 force-pushed the bug18322 branch 2 times, most recently from f965e3f to 1139d94 Compare January 31, 2018 03:09

marler8997 closed this Jan 31, 2018

marler8997 reopened this Jan 31, 2018

JinShil reviewed Feb 1, 2018

View reviewed changes

JinShil approved these changes Feb 1, 2018

View reviewed changes

wilzbach approved these changes Feb 6, 2018

View reviewed changes

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 6, 2018

marler8997 force-pushed the bug18322 branch 2 times, most recently from 172c6a8 to 0062ee5 Compare February 7, 2018 23:22

marler8997 force-pushed the bug18322 branch from 0062ee5 to f9fd665 Compare February 7, 2018 23:26

ibuclaw requested changes Feb 10, 2018

View reviewed changes

ibuclaw removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Feb 10, 2018

ibuclaw reviewed Feb 10, 2018

View reviewed changes

Fix issue 18322

1927521

marler8997 force-pushed the bug18322 branch from f9fd665 to 1927521 Compare February 10, 2018 17:24

ibuclaw approved these changes Feb 10, 2018

View reviewed changes

JinShil added the Merge:auto-merge label Feb 11, 2018

dlang-bot merged commit 6a2db25 into dlang:master Feb 11, 2018

marler8997 deleted the bug18322 branch February 12, 2018 06:22

marler8997 mentioned this pull request Feb 16, 2018

Remove extra field in module.h #7903

Merged

JohanEngelen added a commit to weka/ldc that referenced this pull request May 27, 2018

Backport dlang/dmd#7798 to fix WEKAPP-66130

5192a54

JohanEngelen added a commit to weka/ldc that referenced this pull request May 27, 2018

Backport dlang/dmd#7798 to fix WEKAPP-66130

2bb07d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 18322: __FILE_FULL_PATH__ doesn't work as a template parameter #7798

Fix issue 18322: __FILE_FULL_PATH__ doesn't work as a template parameter #7798

marler8997 commented Jan 28, 2018

dlang-bot commented Jan 28, 2018 •

edited

Loading

wilzbach Jan 28, 2018

marler8997 Jan 29, 2018

wilzbach Jan 28, 2018

marler8997 Jan 29, 2018

wilzbach Jan 28, 2018

wilzbach Jan 28, 2018

marler8997 Jan 29, 2018

wilzbach Jan 28, 2018

marler8997 Jan 29, 2018

marler8997 Jan 29, 2018

ibuclaw commented Jan 29, 2018 •

edited

Loading

marler8997 commented Jan 29, 2018

marler8997 commented Jan 29, 2018 •

edited

Loading

JinShil commented Jan 31, 2018

marler8997 commented Jan 31, 2018

JinShil Feb 1, 2018

timotheecour commented Feb 1, 2018

timotheecour commented Feb 4, 2018

marler8997 commented Feb 5, 2018

wilzbach left a comment

wilzbach Jan 31, 2018

marler8997 Feb 7, 2018

wilzbach Jan 31, 2018

wilzbach Feb 6, 2018

marler8997 Feb 6, 2018

marler8997 commented Feb 7, 2018

ibuclaw Feb 10, 2018

marler8997 Feb 10, 2018

ibuclaw Feb 10, 2018

marler8997 Feb 10, 2018

ibuclaw commented Feb 10, 2018

Fix issue 18322: __FILE_FULL_PATH__ doesn't work as a template parameter #7798

Fix issue 18322: __FILE_FULL_PATH__ doesn't work as a template parameter #7798

Conversation

marler8997 commented Jan 28, 2018

dlang-bot commented Jan 28, 2018 • edited Loading

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

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

ibuclaw commented Jan 29, 2018 • edited Loading

marler8997 commented Jan 29, 2018

marler8997 commented Jan 29, 2018 • edited Loading

JinShil commented Jan 31, 2018

marler8997 commented Jan 31, 2018

Choose a reason for hiding this comment

timotheecour commented Feb 1, 2018

timotheecour commented Feb 4, 2018

marler8997 commented Feb 5, 2018

wilzbach left a 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

marler8997 commented Feb 7, 2018

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

ibuclaw commented Feb 10, 2018

dlang-bot commented Jan 28, 2018 •

edited

Loading

ibuclaw commented Jan 29, 2018 •

edited

Loading

marler8997 commented Jan 29, 2018 •

edited

Loading