Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

deadalnix
Copy link
Contributor

Pull request :
#177

Redone with the correct commit appearing and using alias instead of @Property.

@@ -375,6 +375,19 @@ version( linux )
int si_fd;
} _sigpoll_t _sigpoll;
} _sifields_t _sifields;

// How these fields are to be accessed. (as mentionned in kernel source code).
alias _sifields._kill.si_pid si_pid;
Copy link
Member

Choose a reason for hiding this comment

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

Aliases to nested fields don't work.

@deadalnix
Copy link
Contributor Author

My bad. Back on the original solution.


// How these fields are to be accessed. (as mentionned in kernel source code).
@property
auto si_pid() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for using auto here? Unless there's a semantic reason, please avoid it so the declaration is clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type is defined in the field above. I don't think it is any clearer, simply an old habit.

In kernel's source code, this is done using define. The type is specified nowhere, the important point is that it is a shortcut to a deeper field in the struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

When reading the documentation, I shouldn't have to look at the source code to figure out what it's 1) returning 2) what type the data is that it returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not authoritative on this. It is linux's kernel specific code, and the documentation doesn't belong to us in any ways.

The purpose of this code is to reflect what is specified in linux's doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Link to said doc?

I don't understand why you want these properties to return auto. It's just trying to pretend that they don't have a well-known type, which they do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doc is in "man sigaction" .

auto is much simpler here. such properties are simply shortcut to deeper fields. Any modification to such field would require a modification of the property for no benefit. It could happen for instance to port druntime on new plateforms.

Such property isn't the authoritative place where the type is lying, and so it is duplicating of knowlegde. It is known to be a bad practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dude, it's called static, strong typing. :)

These properties are useless. I can't know anything about the values they give me. I can't serialize them to files, write them to memory, ... or whatever I might want to do. This is different for so-called Voldemort types because they are designed to not have a name. What these properties return are simple primitive values.

Yes, C code using macros is typically designed so that the programmer should assume nothing about the type of the data being operated on. We don't have that 'luxury'. Even if the type of these values changes in some $FUTURE_VERSION of Linux, you have to change the fields anyway, which will break compatibility anyway. Making these properties auto buys you nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you say make no sense.

Theses properties are strongly typed. The property is meant to be a shortcut to a deeper field in the structure. Such shortcut exists, whatever the version of linux, whatever the platform, whatever whatever. They always have the type of the pointed field.

The property isn't the authoritative representation of the type, so they are auto. It have nothing to do with with static typing. It would if I chose to use Variant, but I did use auto, which isn't the same.

It have to do with having only one authoritative representation of knowledge in the system. Or, as stated in pragmatic programmer : « Every piece of knowledge must have a single, unambiguous, authoritative representation within a system. » . This one unambiguous authoritative representation is lying in the variable itself, not in the shortcut to access it.

It have nothing to do with future version of linux, at is is highly unlikely that they break binary compatibility, but it have everything to do with adapting to newer platform (other CPU arch or new ABI linux x32 ).

Copy link
Member

Choose a reason for hiding this comment

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

They way they deal with this in C (where there's no type inference) is by adding typedefs a la sigset_t etc. We should also define those names as aliases and use them in the definition of the properties. @deadalnix I also think using auto here does more harm than good. Thanks!

@MartinNowak
Copy link
Member

  • Having l-value access through return by ref would better match the semantics of a macro
    and it would reduces the amount of code.
  • Could you shrink the functions to one-liners please.
  • FreeBSD needs #define si_band _reason._poll._band
  • Please remove the linux specific extensions
    (si_tid, si_overrun, si_utime, si_stime, si_int, si_ptr).
    opengroup spec
    signal.d comment

@deadalnix
Copy link
Contributor Author

Noted, I'll update that.

@deadalnix
Copy link
Contributor Author

As most people seems to be against the use of auto, I explicited types.

@MartinNowak MartinNowak merged commit d0b6170 into dlang:master Jul 10, 2012
MartinNowak added a commit that referenced this pull request Jul 17, 2012
- need to compile siginfo_t methods
@deadalnix deadalnix mentioned this pull request Sep 17, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants