Return to Answer

• I assume your purpose here is to make the hilbert function faster, because you will call it multiple times. You've moved the FFT plan creation outside of hilbert, which is probably a huge speed increase. However, you still call fftw_destroy_plan within hilbert. This is a problem if you call hilbert more than once. You should move fftw_destroy_plan outside of hilbert, so it would be called from main instead.
• You have three different for loops inside hilbert. One copies data from input vector to output vector. One scales half the output vector by a factor of 2. One scales the output vector by 1/N. These loops can all be combined into a single loop at the beginning of hilbert. This works because an FFT is linear, so scaling the input by a constant factor is the same as scaling the output by a constant factor. This new loop would look something like:

      auto scale = 2.0 / N;
      for (int i = 0; i < N; ++i)
      {
          out[i][REAL] = in[i] * scale;
          out[i][IMAG] = 0.0;
      }
  

  Then you would get rid of the other for loops. You would also get rid of the code which multiplies out[hN] by 2 (but keep the call to memset which zeros the second half of the output). Finally, you need to fix out[0], which is now too large by a factor of two:

      out[0][REAL] /= 2.0;
      out[0][IMAG] /= 2.0;
  

  However, the speed increase from combining these loops is probably small compared to the FFT time.
• It may take a little more work, but you could change the forward FFT so that it takes its input directly from the real-valued input vector. This is probably close to twice as fast as the complex FFT you are currently doing. Still, I think you will find your complex FFT is fast enough now that you remove the creation/destruction of the plan outside of hilbert.

• I assume your purpose here is to make the hilbert function faster, because you will call it multiple times. You've moved the FFT plan creation outside of hilbert, which is probably a huge speed increase. However, you still call fftw_destroy_plan within hilbert. This is a problem if you call hilbert more than once. You should move fftw_destroy_plan outside of hilbert, so it would be called from main instead.
• You have three different for loops inside hilbert. One copies data from input vector to output vector. One scales half the output vector by a factor of 2. One scales the output vector by 1/N. These loops can all be combined into a single loop at the beginning of hilbert. This works because an FFT is linear, so scaling the input by a constant factor is the same as scaling the output by a constant factor. This new loop would look something like:

      auto scale = 2.0 / N;
      for (int i = 0; i < N; ++i)
      {
          out[i][REAL] = in[i] * scale;
          out[i][IMAG] = 0.0;
      }
  

  Then you would get rid of the other for loops. You would also get rid of the code which multiplies out[hN] by 2 (but keep the call to memset which zeros the second half of the output). Finally, you need to fix out[0], which is now too large by a factor of two:

      out[0][REAL] /= 2.0;
      out[0][IMAG] /= 2.0;
  

  However, the speed increase from combining these loops is probably small compared to the FFT time.
• It may take a little more work, but you could change the forward FFT so that it takes its input directly from the real-valued input vector. This is probably close to twice as fast as the complex FFT you are currently doing (but this is just for the forward FFT, so your overall speed increase might be up to 25%). Still, I think it is likely that your complex FFT is already fast enough now that you've removed the creation/destruction of the plan outside of hilbert.

Code Review

Updated Code Review for 2020-12-30 Code

Some of the original code review items have been addressed. Good work! Here are some remaining ideas:

• I assume your purpose here is to make the hilbert function faster, because you will call it multiple times. You've moved the FFT plan creation outside of hilbert, which is probably a huge speed increase. However, you still call fftw_destroy_plan within hilbert. This is a problem if you call hilbert more than once. You should move fftw_destroy_plan outside of hilbert, so it would be called from main instead.
• You have three different for loops inside hilbert. One copies data from input vector to output vector. One scales half the output vector by a factor of 2. One scales the output vector by 1/N. These loops can all be combined into a single loop at the beginning of hilbert. This works because an FFT is linear, so scaling the input by a constant factor is the same as scaling the output by a constant factor. This new loop would look something like:

      auto scale = 2.0 / N;
      for (int i = 0; i < N; ++i)
      {
          out[i][REAL] = in[i] * scale;
          out[i][IMAG] = 0.0;
      }
  

  Then you would get rid of the other for loops. You would also get rid of the code which multiplies out[hN] by 2 (but keep the call to memset which zeros the second half of the output). Finally, you need to fix out[0], which is now too large by a factor of two:

      out[0][REAL] /= 2.0;
      out[0][IMAG] /= 2.0;
  

  However, the speed increase from combining these loops is probably small compared to the FFT time.
• It may take a little more work, but you could change the forward FFT so that it takes its input directly from the real-valued input vector. This is probably close to twice as fast as the complex FFT you are currently doing. Still, I think you will find your complex FFT is fast enough now that you remove the creation/destruction of the plan outside of hilbert.

Original Code Review for Original 2020-12-26 Code

As will most things related to coding style, reasonable people may have differing opinions. Furthermore, nothing is absolute, so I certainly acknowledge that there are cases where global variables are useful or even necessary. Perhaps I should have said "use global variables only when really necessary".

• What do commonly-used coding guidelines say?

  There are lots of coding guidelines on the internet. Many of them in some way discourage the use of global variables. Some examples:

  • The C++ Core Guidelines, maintained by Bjarne Stroustrup and Herb Sutter say to avoid non-const global variables.
  • The Google C++ Style Guide forbids objects with static storage duration unless they are trivially destructable, and also discourages static storage duration if the object uses dynamic initialization. (Note: global variables have static storage duration.)
  • In Joint Strike Fighter C++ Coding Standards, based on the MISRA C++ coding standards, AV Rule 98 says "Every nonlocal name, except main(), should be placed in some namespace." (This doesn't prohibit global variables, but does help avoid some of the problems caused by global variables.)
  • In University of Michigan C++ Coding Standards, global variables are acceptable only in certain limited circumstances.
  • Some random code guideline found on the internet discourages use of global variables, suggesting that you instead use variables declared in functions.

• What can go wrong when you use global variables

  • Initialization issues. If global variables are declared in two different files, there is no specification of the order in which they are initialized. If the initialization of one of them depends on the value of the other, you have undefined behavior. It may appear to work sometimes, but it can't be relied upon.
  • Multi-thread issues. If multiple threads simultaneously try to use or modify a global variable, you can get unexpected or undefined behavior. Making this work correctly can be tricky, sometimes requiring the use of mutexes or similar constructs. Often, these constructs slow down the code substantially.
  • Name conflicts within a file. You can have a function-local variable and a global variable with the same name. They are different variables, and this can lead to confusion. Maybe you meant the local variable to access the global variable, but accidentally declared it locally. Or maybe you meant for the local variable to be different than the global variable, but a subsequent maintainer may not realize that and think they are the same variable. These kind of issues can lead to bugs.
  • Name conflicts with external libraries. If you have an external global variable, it can conflict with other external global variables with the same name. These kind of conflicts can make it difficult to integrate separate software libraries.

As with most things related to coding style, reasonable people may have differing opinions. Furthermore, nothing is absolute, so I certainly acknowledge that there are cases where global variables are useful or even necessary. Perhaps I should have said "use global variables only when really necessary".

• What do commonly-used coding guidelines say?

  There are lots of coding guidelines on the internet. Many of them in some way discourage the use of global variables. Some examples:

  • The C++ Core Guidelines, maintained by Bjarne Stroustrup and Herb Sutter, say to avoid non-const global variables.
  • The Google C++ Style Guide forbids objects with static storage duration unless they are trivially destructable, and also discourages static storage duration if the object uses dynamic initialization. (Note: global variables have static storage duration.)
  • In Joint Strike Fighter C++ Coding Standards, based on the MISRA C++ coding standards, AV Rule 98 says "Every nonlocal name, except main(), should be placed in some namespace." (This doesn't prohibit global variables, but does help avoid some of the problems caused by global variables.)
  • In University of Michigan C++ Coding Standards, global variables are acceptable only in certain limited circumstances.
  • Some random code guideline found on the internet discourages use of global variables, suggesting that you instead use variables declared in functions.

• What can go wrong when you use global variables

  • Initialization issues. If global variables are declared in two different files, there is no specification of the order in which they are initialized. If the initialization of one of them depends on the value of the other, you have undefined behavior. It may appear to work sometimes, but it can't be relied upon.
  • Multi-thread issues. If multiple threads simultaneously try to use or modify a global variable, you can get unexpected or undefined behavior. Making this work correctly can be tricky, sometimes requiring the use of mutexes or similar constructs. Often, these constructs slow down the code substantially.
  • Name conflicts within a file. You can have a function-local variable and a global variable with the same name. They are different variables, and this can lead to confusion. Maybe you meant the local variable to access the global variable, but accidentally declared it locally. Or maybe you meant for the local variable to be different than the global variable, but a subsequent maintainer may not realize that and think they are the same variable. These kind of issues can lead to bugs.
  • Name conflicts with external libraries. If you have an external global variable, it can conflict with other external global variables with the same name. These kind of conflicts can make it difficult to integrate separate software libraries.