A question about the anchor class

[irwir]

Library version 1.3 used CSize for anchors, and version 1.4 introduced a special class tagAnchor. But this new class duplicates properties of CSize. The declaration of tagAnchor class could be replaced with just one line, and it also would keep a meaningful name for anchors: typedef CSize ANCHOR; #define ANCHOR CSize

CPoint class also could be used in the same way, but it has x, y member names instead of cx, cy. Technically, replacement with CPoint could be a breaking change, though I am unaware of any user code that directly manipulated anchor values, so only library code needed adjustments. At least, all my compilations were successful.

[ppescher]

I guess I received a comment that was exactly the opposite as yours, back in the days of CodeProject, and decided to change. I really don't remember, but thinking about it now I can't really say what's better. Pros and cons?

[irwir]

I guess I received a comment that was exactly the opposite as yours

That seems unlikely. The library v1.3 was using CSize for two different kinds of objects, sizes and anchors. Of course, you could be asked to make these objects to be more distinguishable, but I am not suggesting to return to the previous state.

A completely new class for the anchors might be necessary, for example, for range checking of anchor's data members (to be -1 or between 0 and 100%), or for making it a pure data structure (and avoid any construction/destruction code).

Otherwise, if the goal is to have distinct classes for different objects, then implementation still could use the same base class. In that case, typedef is sufficient, and it makes the source shorter too. It might be preferable to switch from CSize to CPoint though, because essentially anchors are points, not sizes.

If you agree with the changes, this could be made a part of my next PR.

[irwir]

Are the given arguments sufficient or not?

[ppescher]

So what is your suggestion? Initially you said to remove ANCHOR implementation an replace it with a typedef, then you said a new class might be necessary... I'm a little confused.

I think anchors should have a distinct type, not CSize nor CPoint. And I don't see what's wrong with the current implementation... are you saying you want to remove it or expand it?

[irwir]

Necessity of a new class was discussed as a requirement for an extended functionality. But there are no such extensions currently. Hence a typedef should be good enough, it takes only one line given above and provides better name (better than CSize or CPoint). CPoint would be more logical than CSize, though it would require replacing cx and cy members with x and y correspondingly. But that touches only two or three places in the library.

[ppescher]

I still don't see it like a necessary thing. Moreover, having distinct types forbids using one for another, that is you cannot pass a CSize or CPoint as an anchor, that you could do if you had a typedef.

[irwir]

having distinct types forbids using one for another

With that logic, should separate classes for window's width and height be considered? People rarely do that. Macros and typedefs would be more common, though numbers could be misplaced and misused.

In case of anchors only a pair of numbers is needed. That is, anchor is a point, and standard CPoint conveniently provides this functionality. Why not use it then? The change cannot be called a necessity, because it was not a bug. But use of standard classes and shorter source code are on the positive side.

[ppescher]

Well I prefer not making changes if not actually necessary. I don't see a big improvement here, after all they are only about ten lines of code so I don't think it makes any difference. Is it really worth it?

[irwir]

Not that simple. :) ResizableLayout.* files need serious changes for another reason, then this class replacement might be only an extra. As for improvement, it is minor indeed, but (1) there is no reason to duplicate exactly the already existing classes (2) no increase in the library size becase the standard class is already used in the library (3) it is possible to press F1 and read description for the class in plain English instead of reading source code (4) though if necessary,. source code for the standard classes is present too.

[ppescher]

I'm having a hard time following your points, especially 3 and 4... and what other serious changes are needed? If they are not related to this "anchor type" issue, then please just leave them in separate commits.

[irwir]

Points 3 and 4 refer to cases when someone needs deeper understanding of the library code. Standard classes are usually better documented, and more familiar to users than custom classes in the library. That is why I prefer to use standard classes when possible. Also standard classes are often (not always) better debugged.

[ppescher]

Generally, I would agree with you, but we're talking about this code:

typedef struct tagANCHOR
{
    int cx; //!< horizontal component, in percent
    int cy; //!< vertical component, in percent

    tagANCHOR() : cx(0), cy(0) {}

    tagANCHOR(int x, int y)
    {
        cx = x;
        cy = y;
    }

} ANCHOR, *PANCHOR, *LPANCHOR;

There's not really much there to understand or debug. Anchors are meant to be instantiated only on the client side, and the library has direct access to its members. I didn't even bother to prevent access from clients with private/friend stuff. It's so simple it's not worth it.

[irwir]

It's so simple it's not worth it.

Simple enough to raise questions: Why it looks familiar, and Are there big reasons to define a new class here? The conclusion was that it would be hard to beat one-liner's simplicity.

[irwir]

To get the idea of the future PR, please take a look at https://github.com/irwir/resizablelib/tree/updates All in one bundle, because this is my current code, and there were no separate commits. Also OnInitDialog() event handler should be moved to public because it was decared as pubic in parent CWnd class. If you will find it necessary to change/split etc. things - I could fix it later.

[ppescher]

I was looking at https://github.com/ppescher/resizablelib/compare/master...irwir:updates The only change I see that might be necessary is OnInitDialog() moved to the public section of CResizablePage, even if I can't imagine a scenario where you need to call it from the outside. The rest are mostly formatting/cosmetic changes. Is the (unsigned) cast actually required? Oh and moving the anchor constants to the source file. What is that actually good for? Is there some optimization involved?

[irwir]

The only change I see that might be necessary is OnInitDialog() moved to the public section of CResizablePage, even if I can't imagine a scenario where you need to call it from the outside.

Scenario is irrelevant. This event handler was declared as public in base class, and scope cannot be reduced in derived classes - it is public still. Trying to do otherwise is diagnosed with warnings. This touches four header files in total, all were added to the commit.

Is the (unsigned) cast actually required?

Integer constant and unsigned formatting character. The cast is the cheapest way to suppress the warning.

Oh and moving the anchor constants to the source file. What is that actually good for? Is there some optimization involved?

While this was not a bug, defining constant values in header files is a really bad practice. These values are duplicated in every file where the header was included. It gets worse for class instances because of implicit construction and destruction code - again in every module. For me the result of this simple move was 8 KB of executable's size reduction (I did not expect that much).

There are also microoptimizatons: define only WINDOWPLACEMENT structure's length and skip filling the remaining part with zeros because it would be overwritten with GetWindowPlacement call.

The rest of the changes, such as const qualifiers, white spaces are cosmetics indeed.

[ppescher]

Code size reduction is a good point. I thought constants defined in header files could be better optimized by the compiler.

I was thinking about the ANCHOR type and maybe it could be interesting to see if any "better" code (smaller/faster) would be generated if that type would fit into 32 bits. Could the POINTS structure be a "better" match in this regard (of course by adding a derived class with appropriate constructor)? Would it be passed as a single 32 bit argument?

Probably just a silly question, but I was intrigued by the idea. It wouldn't have any noticeable improvement or performance gain, but still it would kind of justify the change, at least for me. :-)

[irwir]

I thought constants defined in header files could be better optimized by the compiler.

For independent translation units, how to tell if the programmer wanted to duplicate values or not?

Could the POINTS structure be a "better" match in this regard (of course by adding a derived class with appropriate constructor)?

I considered plain structure and no code, but older VS versions would not compile expressions such as ANCHOR{x, y}. Therefore constuctor(s) should stay for convenience and compatibility.

Would it be passed as a single 32 bit argument?

Memory and/or execution time savings for short integers depend on combination of use case and platform. For example, VS compiler will use movsx instruction to expand short to int before multiplication (see ResizableLayout.cpp code). This adds 3 bytes of code and 1 instruction execution time.

[ppescher]

I did some tests with POINTS and concluded it was a bad idea (code size increased considerably).

However I recall why I had defined all those "structs" and that's because I had the (silly) idea to convert the basic layout code to a C version to be used with Win32 SDK, and then base the MFC code on that one.

I no longer have that idea (lost in time I guess), but really I don't see any improvement in changing ANCHOR to a CPoint typedef (not yet at least). I prefer to keep it like it is, without using MFC classes inside the tagLAYOUT struct.

However I do agree in moving constants to the cpp file. That could save a few bytes.

Would you do a PR of all the other changes, except the ANCHOR typedef?

[irwir]

I did some tests with POINTS and concluded it was a bad idea (code size increased considerably).

Maybe I recall incorrectly, but when playing with structures vs structures+constructor, executable size was the same in optimized release builds (rounded to 512 byte blocks, of course).

I no longer have that idea (lost in time I guess), but really I don't see any improvement in changing ANCHOR to a CPoint typedef (not yet at least). I prefer to keep it like it is, without using MFC classes inside the tagLAYOUT struct.

The library is C++/MFC, and I cannot imagine how C-like structures could make things better.

Would you do a PR of all the other changes, except the ANCHOR typedef?

In my point of view there should be no real issues with another instance of already used class and slightly shortened source code. But I will revert to tagLAYOUT unless you have got second thoughts on the subject in the time passed. :)

[ppescher]

Thanks @irwir The time has passed but I guess my thoughts were elsewhere, especially now, even if staying home should leave time for thought.

Anyway, my point of view on the ANCHOR type has not changed in the meanwhile. But be assured that I appreciate your contributions, so please don't stop. :-)

By the way, I was trying to use CDialogEx and build a CResizableDialogEx demo project along the way, but I found myself a bit rusty with the latest VisualStudio. Making all the build configurations with Debug/Release, Unicode/ANSI, Win32/x64 is quite a challenge for me, not doing this stuff since a long long time ago. So I don't expect to finish that any time soon.

[irwir]

Pull request created. Hope you will be doing well, and there will be spare time to build up skills of handling Visual Studio.