Closed Bug 1885695 Opened 2 months ago Closed 1 month ago

Convert LogicalEdge to enum class in WritingMode.h

Categories

(Core :: Layout, task)

Product:
Core
Component:
Layout
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
127 Branch
Tracking Flags:
Tracking Status
firefox127 --- fixed

People

(Reporter: TYLin, Assigned: laxtennis2013, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=C++] )

Attachments

(1 file, 3 obsolete files)

Bug 1885695 - Change LogicalEdge to an enum class and change its variants names. r=TYLin
48 bytes, text/x-phabricator-request
Details | Review
Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Attachment #9391577 - Attachment is obsolete: true
Assignee: aethanyc → nobody
Status: ASSIGNED → NEW
Keywords: good-first-bug

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #0)

LogicalEdge is still an old-style enum. Let's convert it to an enum class to improve type safety.
https://searchfox.org/mozilla-central/rev/5f0a7ca8968ac5cef8846e1d970ef178b8b76dcc/layout/generic/WritingModes.h#53

See https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html#variable-prefixes for the coding style for enum class.

I think I can take on this bug. This is my first contribution so bear with me : ).
All i'm doing is changing the declaration to "enum class" correct?

Flags: needinfo?(aethanyc)

I think I can take on this bug. This is my first contribution so bear with me : ).

Nice! You can start from https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, and once you attach the patch, you'll be assigned to this bug.

All i'm doing is changing the declaration to "enum class" correct?

Yes. The patch should be similar to bug bug 1885691.

Flags: needinfo?(aethanyc)

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #3)

I think I can take on this bug. This is my first contribution so bear with me : ).

Nice! You can start from https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html, and once you attach the patch, you'll be assigned to this bug.

All i'm doing is changing the declaration to "enum class" correct?

Yes. The patch should be similar to bug bug 1885691.

Thank you! So right off the bat, I tried pulling before making any changes but I am having an error when attempting to pull. I get a message saying :
git: 'remote-hg' is not a git command. See 'git --help'.

The most similar commands are
remote-fd
remote-http

I should have all dependencies installed, so I am not sure what might be the issue. Thank you in advance.

I guess you've completed bootstrap step in https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-setup-firefox-on-macos-and-linux

Firefox codebase, i.e. mozilla-central is a mercurial repository, so you'd need to use hg pull to pull remote commits. It is still possible to use git, but the setup is more involved.

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #5)

I guess you've completed bootstrap step in https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-setup-firefox-on-macos-and-linux

Firefox codebase, i.e. mozilla-central is a mercurial repository, so you'd need to use hg pull to pull remote commits. It is still possible to use git, but the setup is more involved.

Ah ok. So when I use any command with hg, it I get ' bash: hg.exe: command not found ' but when I try to install mercurial with pip install Mercirial it outputs that ' Requirement already satisfied: Mercurial in c:\mozilla-build\python3\lib\site-packages (6.7) '. So I have it installed but it is not recognizing it. am i missing something?

Looks like the installed hg.exe is not in the path. Sorry I don't use Windows, so I don't have insight to recommend the next step.

Mozilla matrix https://chat.mozilla.org has a #introduction channel for helping with basic developing for Mozilla. There must be some developers using Windows that might be able to help. https://wiki.mozilla.org/Matrix#New_to_Matrix.2C_new_to_Mozilla.3F

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #7)

Looks like the installed hg.exe is not in the path. Sorry I don't use Windows, so I don't have insight to recommend the next step.

Mozilla matrix https://chat.mozilla.org has a #introduction channel for helping with basic developing for Mozilla. There must be some developers using Windows that might be able to help. https://wiki.mozilla.org/Matrix#New_to_Matrix.2C_new_to_Mozilla.3F

Thank you, I think I found a "workaround' by just reinstalling. However, I now am trying to hg pull but it says no repository found in mozilla-unified. Is mozilla-unified not the working directory? I'm looking up solutions and some suggestions say to use hg init (which I don't know if that messes up the directory for later pushes) or reinstall the source code. sorry for this inconvenience and all these silly issues I'm having

Could you give the commands in "To Setup Firefox On Windows" another try? After python3 bootstrap.py is done, it should give you a fully functional mozilla-unified repository under the directory where you run the command.

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #9)

Could you give the commands in "To Setup Firefox On Windows" another try? After python3 bootstrap.py is done, it should give you a fully functional mozilla-unified repository under the directory where you run the command.

Alright so I tried that an it works now, thank you! However, now I am trying test if the change from enum to enum class works by building and then ./mach run, but when I try to build the build fails. I thought that this bug was for C++ but it is a Rust problem so I might not know of some fundamental knowledge I may need here, but I thought that all I had to type was 'class' after 'enum' for LogicalEdge. I am wondering if I misunderstood the assignment because nothing I look up says is helping me with my error unfortunately. But another things that doesn't seem right is that when I successfully build it keeps taking more disk space. Is there a way to stop that, I'm afraid I might run out of disk space by running ./mach build every time I want to run the build after I make a change.

Do you mind sharing the build error message? Also, you might want to try rustup update to make sure the installed rust is updated.

I thought that all I had to type was 'class' after 'enum' for LogicalEdge

You'll also need to change eLogicalEdgeStart to EdgeStart Start and eLogicalEdgeEnd to EdgeEnd End to match the coding style. Of course, the callsites will have to change, too.

[edit] Sorry that I made a wrong suggestion. the new enum variants should be Start and End since the enum name is LogicalEdge. No need to repeat the Edge.

If you've made a successful build before making any change, I think ./mach build won't consume too many space after you make a simple change like changing enum to enum class. What is the amount of disk space increase you were seeing after each ./mach build?

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #11)

Do you mind sharing the build error message? Also, you might want to try rustup update to make sure the installed rust is updated.

You'll also need to change eLogicalEdgeStart to EdgeStart and eLogicalEdgeEnd to EdgeEnd match the coding style. Of course, the callsites will have to change, too.

If you've made a successful build before making any change, I think ./mach build won't consume too many space after you make a simple change like changing enum to enum class. What is the amount of disk space increase you were seeing after each ./mach build?

The error message was this:
0:40.34 warning: build failed, waiting for other jobs to finish...
0:48.75 mozmake[4]: *** [C:/mozilla-source/mozilla-unified/config/makefiles/rus
t.mk:498: force-cargo-library-build] Error 101
0:48.75 mozmake[3]: *** [C:/mozilla-source/mozilla-unified/config/recurse.mk:72
: toolkit/library/rust/target-objects] Error 2
0:48.75 mozmake[2]: *** [C:/mozilla-source/mozilla-unified/config/recurse.mk:34
: compile] Error 2
0:48.75 mozmake[1]: *** [C:/mozilla-source/mozilla-unified/config/rules.mk:361:
default] Error 2
0:48.75 mozmake: *** [client.mk:60: build] Error 2
0:48.79 W 160 compiler warnings present

I ran rustup update and it is up to date. I'll change the callsites and and everything else you mentioned in the meantime.
My disk space when from 425GB left to 417GB left and now 410 GB left. I am not sure if it is just the build doing that so I will look into other causes as well. Thank you

Hmm, I fail to guess any reason that might cause the build error. I'd try ./mach clobber to clear any intermediate file to restore the repo to a clean state, and try ./mach build again. That might also free the disk space a bit.

(In reply to Ting-Yu Lin [:TYLin] (PDT, UTC-7) from comment #13)

Hmm, I fail to guess any reason that might cause the build error. I'd try ./mach clobber to clear any intermediate file to restore the repo to a clean state, and try ./mach build again. That might also free the disk space a bit.

So I scrolled up a lot more to see what the other error messages where and it seems a bit more clear what it is having an issue with. At compile time most of the errors seem to say something along the lines of :
3:12.67 C:/mozilla-source/mozilla-unified/obj-x86_64-pc-windows-msvc/dist/include/mozilla/Writi
ngModes.h(55,49): error: invalid operands to binary expression ('int' and 'mozilla::LogicalEdge'
)
3:12.67 55 | eLogicalSideBStart = (eLogicalAxisBlock << 1) | LogicalEdge::EdgeStart, //
0x0
3:12.67 | ~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~

So they start to recognize EdgeStart as the enum class but they aren't recognizing the number that is assigned to EdgeStart.
Is there a reason for that and is there a way to type cast it?

I don't think there is a reason for that. Coincidentally, :flowejam is tweaking that line in https://phabricator.services.mozilla.com/D205510 (bug 1885693), and that patch might simplify your work here.

I see I'll wait for his patch to go through. Also I realized that I haven't been assigned for this bug and I know how but I am a little confused. From the instructions online it seemed like I already had to have committed a change made to submit a patch but I cannot get past the build error part so that I can push an error-less change. I think I am doing the order of the steps wrong.

I'm making this bug depend on bug 1885695 so that once bug 1885695 hits mozilla-central, everyone cced in this bug will get notification.

Depends on: 1885693

Is this bug still open to be fixed ?
If yes, I would like to be assigned for the task.

(In reply to siddharthaswarnkar from comment #18)

Is this bug still open to be fixed ?
If yes, I would like to be assigned for the task.

Hi siddharthaswarnkar. Yes, I have actually been working on this bug for a couple of days now but haven't been able to finish it yet due to it depending on another bug.

Hey :TYLin I have pulled the changes from the other patch but now when I try to build the errors are a little different. The errors are now about an undeclared identifier in different files though. I'll paste two error messages to show a few(all of them are the same error but at different lines and files):
4:50.12 C:/mozilla-source/mozilla-unified/layout/generic/CSSAlignUtils.cpp(59,39): error: use o
f undeclared identifier 'eLogicalEdgeStart'
4:50.13 59 | aAxis, MOZ_LIKELY(isSameSide) ? eLogicalEdgeStart : eLogicalEdgeEnd);
4:50.13 | ^
4:50.13 C:/mozilla-source/mozilla-unified/layout/generic/CSSAlignUtils.cpp(59,59): error: use o
f undeclared identifier 'eLogicalEdgeEnd'
4:50.13 59 | aAxis, MOZ_LIKELY(isSameSide) ? eLogicalEdgeStart : eLogicalEdgeEnd);
4:50.13 | ^

Am I allowed to change the identifier names in the other files, they must rely on WritingModes.h?

Yes, if you've followed my direction in comment 11 to change the enum variants to EdgeStart etc, all the callers in other files must change accordingly.

If you still encounter build errors, you might want to submit a patch even if it is still work-in-progress, I can better see what is going on on my end.

Ok thank you I will do that

Assignee: nobody → laxtennis2013
Status: NEW → ASSIGNED

Hey TYLin for your comment about my repo not being up to date, I wanted to let you know that I tried pulling from central(hg pull central and hg pull), however, I am not seeing any change. Am I using the wrong command?

hg update or hg update central should update the working directory.

to
Start and End, however, errors occured with variable aEdge because it is not being seen as an integer when it needs to be. r=TYLin

Got it thank you. I tried making the changes again but I got the same errors on build, messages saying:

2:35.45 C:/mozilla-source/mozilla-unified/obj-x86_64-pc-windows-msvc/dist/inclu
de/mozilla/WritingModes.h(112,24): error: invalid operands to binary expression
('int' and 'LogicalEdge')
2:35.45 112 | return LogicalEdge(1 - aEdge);
2:35.45 | ~ ^ ~~~~~

I am not sure how it works all too well, but how would the parameter aEdge know to pick Start or End? Is that what is maybe causing it not to be recognized as an int?

There are build errors because an enum class does not automatically convert to an integer like an enum, so we have to do type-casting explicitly. See my inline comments for the fix https://phabricator.services.mozilla.com/D206265#7078929

Attachment #9394291 - Attachment description: WIP: Bug 1885695 - changed LogicalEdge to enum class and its object's names → WIP: Bug 1885695 - changed LogicalEdge to enum class and its object's

Hi TYLin, I commited the appropriate changes but I'm not sure if they did go through on the same commit. Does everything seems fine with my push?

I've reviewed your latest revision in https://phabricator.services.mozilla.com/D206265, but there are still some suggestions I feel like we should address. Please see my comments. Thanks!

Flags: needinfo?(laxtennis2013)

jmc531, feel free to let me know if you need other help on this bug. If you are occupied recently, I can also take over and bring your patch across the finish line.

Attachment #9394291 - Attachment description: WIP: Bug 1885695 - changed LogicalEdge to enum class and its object's → WIP: Bug 1885695 - LogicalEdge now enum class, changed its objects names. r=TYLin

Sorry for my delay in getting this patch done! Let me know if there is anything else I missed or I didn't do correctly. Thank you.

Flags: needinfo?(laxtennis2013)

The latest revision in https://phabricator.services.mozilla.com/D206265 looks good. Could you use phabricator's interface (near the comment box) "Add Action ..." -> "Request Review" so that I can accept and land the patch.

Also, please use "Add action ..." -> "Abandon Revision" to discard https://phabricator.services.mozilla.com/D206118 and https://phabricator.services.mozilla.com/D206109 so that other people won't be confused by obsoleted patches while reading this bug.

Flags: needinfo?(laxtennis2013)
Attachment #9393988 - Attachment is obsolete: true
Attachment #9393971 - Attachment is obsolete: true

I think I did what your comments suggested. let me know if it didn't go through or if there is anything else i need to do. thanks

Flags: needinfo?(laxtennis2013)
Attachment #9394291 - Attachment description: WIP: Bug 1885695 - LogicalEdge now enum class, changed its objects names. r=TYLin → Bug 1885695 - Change LogicalEdge to an enum class and change its variants names. r=TYLin
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a9ba5e4d11bb
Change LogicalEdge to an enum class and change its variants names. r=TYLin

https://hg.mozilla.org/mozilla-central/rev/a9ba5e4d11bb

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
status-firefox127: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 127 Branch

let me know if it didn't go through or if there is anything else i need to do. thanks

Congrats on landing your first patch to Firefox!

Thank you! This was a fun learning experience. Sorry if this took longer than what is should have. Thank you again for all your help.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: