Page MenuHome

Code quality: Enable SortedIncludes
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Feb 11 2020, 2:04 PM.

Details

Summary

Code quality: Enable SortedIncludes in .clang-format

This patch does not include a make format, which will follow suit.

Diff Detail

Repository
rB Blender

Event Timeline

Header sorting is not going to work for the Cycles kernel. It's used to compile the entire kernel as a single file, and the order there matters.

I guess we can sprinkle some /* clang-format off */ in the code there, unless there is a way to disable this for entire directories.

Fixed for ghost, and sustom .clang-format without SortedInclude for Cycles and Freestyle

If it's just a handful of files, /* clang-format off */ seems preferable over copying the entire configuration.

Fix for Mac building

I was updating the patch based on buildbot and missed your replies.
I will try /* clang-format off */ here and there, good call.

Freestyle /* clang-format off */

Final rebase now that the changes are on master

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)Mar 6 2020, 5:58 PM
Dalai Felinto (dfelinto) retitled this revision from Code quality: Enable SortedIncludes (work in progress) to Code quality: Enable SortedIncludes.Mar 16 2020, 6:18 PM
Ray molenkamp (LazyDodo) requested changes to this revision.EditedMar 16 2020, 7:07 PM

This is currently making a lot of problems for windows, BLI_winstuff.h #defines fseek/tell and friends as 64 bit wrappers, thing is if you include a windows header after BLI_winstuff.h it gets real unhappy about that that fact.

Quick fix : tell clang-format not to mess with the order
Proper fix : Stop using ftell/seek/etc directly in blender and wrap them in a proper BLI_ftell/seek/etc that is 64 bit

I'll happily do the work, but i'm gonna have to know what direction we want to take here.

This revision now requires changes to proceed.Mar 16 2020, 7:07 PM

The simplest solution is just to add an empty line and put BLI_winstuff.h at the top, which I think is acceptable.

#include "BLI_winstuff.h"

#include "BLI_a.h"
#include "BLI_b.h"
...

Adding BLI_ftell and similar would be even better, if it can be done without delaying this patch more than a week then I would try that.

I have a patch ready for the file functions (will get that posted tomorrow), but ran into a second issue that must be resolved HKEY we define it, the windows headers define it and we have been fighting with it ever since, i'm not sure why nobody ever renamed these in enum members

we have in a single enum

EVENT_NONE = 0x0000,
LEFTMOUSE = 0x0001,
WM_IME_COMPOSITE_START = 0x0014,
TABLET_STYLUS = 0x001a,
ZEROKEY = 0x0030,  /* '0' (48). */
HKEY = 0x0068, /* 'h' (104). */
NDOF_MOTION = 0x0190, /* 400 */
EVT_ACTIONZONE_AREA = 0x5000,       /* 20480 */

it kinda feels like we have we just given up here?

To get this patch rushed in (although I don't understand why we are suddenly rushing this) we can probably get away by just renaming the HKEY to perhaps BWM_HKEY (WM_* is out of the question, that's just more collisions with the win32 api waiting to happen) but this be a good candidate for a code quality friday.

Also i'm gonna just call this, none of the issues i am running into are due to recent code changes, all these are things that have been real fragile for a long long time, there is a 0% chance this diff was ever tested on windows, unless there is a real good reason not to deal with the underlying issues, i'd really prefer stalling this patch until these issues (and whatever may still come up) are properly dealt with.

Hi Ray, thanks for looking at that.

Although I don't understand why we are suddenly rushing this

This will always be disruptive. bcon2 is a moment where at least the main big patches that were on the verge to be committed are already in. So just trying to be the least disruptive as possible as far as merge conflicts go. Specially because of LTS I would rather see this in 2.83.

We can probably get away by just renaming the HKEY to perhaps BWM_HKEY (WM_* is out of the question, that's just more collisions with the win32 api waiting to happen) but this be a good candidate for a code quality friday. (...) Unless there is a real good reason not to deal with the underlying issues, i'd really prefer stalling this patch until these issues (and whatever may still come up) are properly dealt with.

I would rather not wait a whole 3 weeks to fix that. If it is really only renaming I can take an hour or two aside and tackle that myself.

On having tested this on Windows.

I did set it to build, but maybe it was when buildbot was failing due to code signature. Anyways, as you pointed out, I clearly haven't gone through the proper testing since you are running into these.

Rebase after openxr fix

I have not tested the latest XR fixes, but beyond that windows looks like its ready to go.

This revision is now accepted and ready to land.Mar 18 2020, 9:21 PM
This revision was automatically updated to reflect the committed changes.