8007 want sys/stddef.h for offsetof and container_of macros

Review Request #417 — Created March 28, 2017 and submitted

tsoome
illumos-gate
8007
d2f3355...
general

8007 want sys/stddef.h for offsetof and container_of macros



  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
yuripv
  1. 
      
  2. usr/src/uts/common/sys/sysmacros.h (Diff revision 1)
     
     

    Shouldn't these two be parenthesized as well?

  3. 
      
tsoome
yuripv
  1. Ship It!
  2. 
      
tsoome
yuripv
  1. Ship It!
  2. 
      
gwr
  1. Dboot is being built "like a boot program" (the Makefiles pass -D_BOOT)
    and it's using stand-alone versions of string functions etc.
    Therefore, I'd recommend that dboot use (one of) the "stand alone"
    headers for this, i.e. $SRC/stand/lib/sa/stddef.h
    What dboot sources current include sysmacros.h?

    1. Currently none is using sysmacros.h. Also using $SRC/stand/lib/sa/ is out of the question, its created for old bootblocks, its not even used in any of the current build - meaning its at least 12 years old, and adjusting it for kernel use is much more work than it it is worth. Even that pointed stddef.h there is not updated for gcc. dboot is using sources from i86pc/dboot and i86pc/boot - the boot is also used by kernel itself, I see no way to start mixing kernel and non-kernel headers.

    2. Can you point me to something that does not build without the proposed change to sysmacros.h?

    3. What you mean? I did post the sample to issue page. From current source you can not find anything like that by definition - otherwise we would not have buildable source.

  2. 
      
tsoome
jbk
  1. 
      
  2. usr/src/uts/common/sys/sysmacros.h (Diff revision 4)
     
     

    Extra paren?

  3. 
      
tsoome
tsoome
tsoome
tsoome
jeffpc
  1. 
      
  2. usr/src/uts/common/sys/stddef.h (Diff revision 7)
     
     

    I'd rather see NULL used instead of 0.

  3. usr/src/uts/common/sys/stddef.h (Diff revision 7)
     
     

    The definition you removed from stddef.h had a C++ version that used std::size_t. Is that still needed?

    1. Unless I got it entirely wrong, I did place the #include <sys/stddef.h> into head/stddef.h after the section:

      if __cplusplus >= 199711L

      using std::ptrdiff_t;
      using std::size_t;

      endif

      include <sys/stddef.h>

      in hope this will do.

    2. I think you are right that it should work.

      Now that you pointed it out, I think I'd rather see all the includes in one clump (above the using lines) and explicitly have a C++ version. It looks like a tempting cleanup to move the sys/stddef.h include up to be with its "friends".

  4. 
      
tsoome
gwr
  1. I appreciate what you're trying to do here. It's good work.
    However, you're taking a "hard path" because with this
    approach you're going to have to figure out whether this
    set of changes breaks any applications (i.e. in pkgsrc).

    If you don't have the time and energy to do that, you
    might want to reconsider one of the earlier suggestions
    such as adding: usr/src/boot/include/stddefs.h

    (The old boot/standalone had its own stddef.h too)

    1. (Though I guess that wouldn't help dboot.)
      Looks fine, but you're gonna need test help.

  2. 
      
gwr
  1. 
      
  2. usr/src/head/stddef.h (Diff revision 8)
     
     

    Any reason this is not with the other includes?

    1. The idea with current variant posted here was to keep sys/stddef.h instance clean from c++, so I did put this include line after "use" statements. However while discussing this same topic with Jeff I did realize we actually have no need - we can just as well just have the c++ invariant in the sys/stddef.h (because kernel build does not use c++ anyhow), and then we can bundle include lines together. This change is not yet pushed as I wanted to see other peoples views and opinions first - now I have one more;)

    2. OK, thanks for the explanation.

  3. 
      
tsoome
tsoome
jeffpc
  1. Ship It!
  2. 
      
tsoome
andy_js
  1. A good improvement but I wonder if we are missing some things:
    http://pubs.opengroup.org/onlinepubs/009695399/basedefs/stddef.h.html

    1. I am most certain this can be improved - just that right now I want to conclude the offsetoff/container_of issue:) I mean, I think we get small improvement, there can be other improvements, but I am happy to let other people to think on next issues:)

      For the link above - the head/stddef.h does provide quite many things, if we should move some more into sys/stddef.h, should be discussed but I think it is an topic for next issue.

    2. I'm with Toomas on this one. We aren't promising full Open Group stddef.h compliance with this fix.

    3. Right. The other stuff mentioned in the Open Group page probably belongs in <stddef.h> not <sys/stddef.h> anyway.
      (Yes, different bug.)

  2. 
      
danmcd
  1. Ship It!
  2. 
      
andy_js
  1. Ship It!
  2. 
      
jeffpc
  1. Ship It!
  2. 
      
gwr
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...