[petsc-dev] Preprocessor hell: #define VecType

[Barry Smith]

  I don't like having two typedefs

  I tried 

typedef const char* VecType;
PETSC_EXTERN PetscErrorCode VecSetType(Vec, const VecType);
PETSC_EXTERN PetscErrorCode VecGetType(Vec, VecType *);

then everything compiled fine except I need to change in a bunch of places

ierr = PetscStrallocpy(ctype,(char**)&da->vectype);CHKERRQ(ierr);

  I can live with this interface (that is requiring some casting when manipulating as strings). Does anyone see problems with this approach?  I know I was opposed to this before but I've come around.



   Barry




On Sep 28, 2012, at 6:16 PM, Karl Rupp <rupp at mcs.anl.gov> wrote:

> Hi,
> 
>>     The problem is we/people may want to "build" XXXType values on the fly with string operations like strcpy, strcat etc. You cannot do that into a const char*, thus we/people would have to declare the place they build things as char* instead of XXXType and that is "unnatural".  Keep digging; we all agree with you that it would be good to get rid of the #define.
>> 
>>     I view this problem as a slight "flaw" in typedef, but perhaps C typedef has a solution?
>> 
>>    Barry
>> 
> 
> okay, this explains the 'why', thanks. :-)
> 
> I've played a bit with the options we have and finally came up with the following snippet, which  resembles the XYZSetType() and XYZGetType() functions currently in use:
> 
> #include "stdio.h"
> #include "stdlib.h"
> 
> #define VecType   char*
> #define VECSHARED "shared"
> 
> int SetType(const VecType v){
> printf("%c\n", v[0]);
> return 0;
> }
> int GetType(const VecType* v){
>  static char bla[] = "bla";
>  *v = bla;
>  return 1;
> }
> 
> int main(int argc, char **argv){
>  const char test1[] = "test1";
>  char test2[] = "test2";
>  const char* test3 = "test3";
>  VecType test4 = (VecType)malloc(5*sizeof(char));
>  const VecType test5 = test4;
> 
>  test4[0] = 'a';
> 
>  printf("-- set --\n");
>  SetType(test1);
>  SetType(test2);
>  SetType(test3);
>  SetType(test4);
>  SetType(test5);
>  SetType(VECSHARED);
> 
>  printf("-- get --\n");
>  /*GetType(&test4);  <-- does not compile with #define
>  printf("%c\n", test4[0]); */
>  GetType(&test5);
>  printf("%c\n", test5[0]);
> 
>  return 0;
> }
> 
> I note the following
> ==> It compiles cleanly
> ==> GetType() expects a pointer to pointer to const. Well, this is probably not overly intuitive, as this suggests that the internal representation is a pointer to const char, while it is actually a char* (see petsc-private/petscimpl.h if I'm not mistaken...). Moreover, in order to call the getter function, one needs to write
>  const VecType type;
>  GetType(&type);
> (see src/vec/vec/impls/nest/vecnest.c) or even the undesired
>  const char *tname;
>  *ierr = GetType(&tname);
> (see src/vec/vec/interface/ftn-custom/zvecregf.c). Hence, while people will usually expect to instantiate a VecType and pass it to GetType(), they have to actually instantiate a 'const VecType' and pass it to GetType in order to get it modified appropriately (huh?).
> 
> 
> Now let's consider the options we have:
> a) typedef char* VecType;  no longer compiles cleanly, as Barry pointed out.
> b) typedef const char* VecType;  imposes usability restrictions and would hence break a lot of user-code.
> c) Keep it as-is. This compiles cleanly, yet the GetType() issue is not intuitive. Also, the Sword of Damocles (here: the preprocessor define) is a constant threat to other code...
> d) Consider the following modification: Instead of a single preprocessor-define, use
>  typedef       char* VecType;
>  typedef const char* VecType_;
> and use the function headers
>  int SetType(const VecType_ v);
>  int GetType(VecType * v)
> This allows for a more intuitive const-correctness, i.e.
>  VecType type; GetType(&type);
> and gets rid of the C preprocessor issue. Also, the reason for the trailing underscore can be explained rather nicely in the documentation and should not cause confusion: Even if a user writes
>  VecType_ new_type = "shared"; SetType(new_type);
> the code remains valid.
> 
> So, even if option d) is still not the cleanest solution we could dream of, I think it is preferable over the current state.
> 
> Best regards,
> Karli
> 
>