NO namespace/file-level includes

[Bob Carpenter]

Please don't put includes at the namespace or
file level. It pollutes the entire namespace.

Daniel and I are taking one set of these out of
lkj_corr.hpp right now and will try to hunt down any
others, but there are a lot of using statements in
Stan.

Does anyone know another way to hunt for spurious
includes?

- Bob

[Ben Goodrich]

On Tuesday, April 9, 2013 3:37:52 PM UTC-4, Bob Carpenter wrote:

Does anyone know another way to hunt for spurious
includes?

https://groups.google.com/d/msg/stan-dev/q977l_VE2Ls/0spLJnlceXIJ

Ben

[Bob Carpenter]

Sorry -- I meant spurious "usings", not spurious "includes".
Just grepping, it's hard to see the context of the using statements
and I want to check we don't have any outside of function
scopes.

Getting rid of spurious includes is good, too.
We should run this again and clean up now that Daniel's
refactored so much of the math lib.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Ben Goodrich]

On Tuesday, April 9, 2013 6:01:43 PM UTC-4, Bob Carpenter wrote:

Sorry -- I meant spurious "usings", not spurious "includes".
Just grepping, it's hard to see the context of the using statements
and I want to check we don't have any outside of function
scopes.

The --show-function option helps a bit

git grep -n -F --show-function "using" src/stan | cat

Ben

[Ben Goodrich]

On Tuesday, April 9, 2013 6:01:43 PM UTC-4, Bob Carpenter wrote:

Sorry -- I meant spurious "usings", not spurious "includes".
Just grepping, it's hard to see the context of the using statements
and I want to check we don't have any outside of function
scopes.

Getting rid of spurious includes is good, too.
We should run this again and clean up now that Daniel's
refactored so much of the math lib.

I was getting ready to run this tool again. But first I ran into an issue that confused me. I had thought that

goodrich@CYBERPOWERPC:/opt/stan$ find src/stan -name '*.hpp' -print | xargs -n 1 -P 7 clang++ -x c++-header -I src -I lib/eigen_3.1.2/ -I lib/boost_1.53.0/ -Wall -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS 2> /tmp/pch_errors.txt

would succeed in making a precompiled header out of every .hpp file in stan/src/. Instead, there are many errors (attached) that do not show up in our file_include tests.  So, what is the difference between a file_include test and the ability to make a precompiled header out of each header, aside from the fact that we currently have 402 .hpp files and only 89 files under src/test/file_includes ?

Ben

[Ben Goodrich]

Really attached file.

Ben
 

[Ben Goodrich]

On Saturday, April 13, 2013 5:57:02 PM UTC-4, Ben Goodrich wrote:

I was getting ready to run this tool again. But first I ran into an issue that confused me. I had thought that

goodrich@CYBERPOWERPC:/opt/stan$ find src/stan -name '*.hpp' -print | xargs -n 1 -P 7 clang++ -x c++-header -I src -I lib/eigen_3.1.2/ -I lib/boost_1.53.0/ -Wall -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS 2> /tmp/pch_errors.txt


would succeed in making a precompiled header out of every .hpp file in stan/src/. Instead, there are many errors (attached) that do not show up in our file_include tests.

Upon further review, many of these failures seem to be buggy code or at least inconsistent with the "include-what-you-use" principle. So, maybe the above should be the test rather than all those file_include tests? In any event, I've started fixing them, but somebody might want to double check me and look into the ones below that I didn't fix:

src/stan/math/matrix/typedefs.hpp:9:66: error: no type named 'size_type' in 'Eigen::Matrix<double, -1, -1, 0, -1, -1>'
    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;

git show 5911afb
...
-    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;
+    typedef size_t size_type;

src/stan/memory/chunk_alloc.hpp:34:18: error: use of undeclared identifier 'eight_byte_aligned_malloc'
      blocks_(1, eight_byte_aligned_malloc(sizeof(T)*Tnchunks_per_block)),

git show a6bc07e
...
+#include <stan/memory/stack_alloc.hpp>

src/stan/agrad/hessian.hpp:7:10: fatal error: 'stan/agrad/fvar.hpp' file not found
#include <stan/agrad/fvar.hpp>

Not fixed but hessian.hpp shouldn't even be in the master branch yet

src/stan/mcmc/nuts_nondiag.hpp:73:21: error: no member named 'sub' in namespace 'stan::math'; did you mean 'sum'?
        stan::math::sub(xplus, xminus, total_direction);

git show 456629d
...
+#include <stan/math/functions/sub.hpp>


src/stan/mcmc/nuts_nondiag.hpp:165:28: error: no matching function for call to 'dot_self'
        double H0 = -0.5 * stan::math::dot_self(mminus) + this->_logp;

git show 4c9a4ab
...
+#include <stan/math/functions/dot_self.hpp>

src/stan/mcmc/nuts_nondiag.hpp:389:22: error: no matching function for call to 'min'
          prob_sum = stan::math::min(1, exp(newH - H0));

git show 95b4dee
...
-          prob_sum = stan::math::min(1, exp(newH - H0));
+          prob_sum = exp(newH - H0);
+          if(prob_sum > 1)
+            prob_sum = 1;

src/stan/mcmc/nuts_massgiven.hpp:71:25: error: no member named 'sub' in namespace 'stan::math'; did you mean 'sum'?
            stan::math::sub(xplus, xminus, total_direction);

Same fix as 456629d

src/stan/mcmc/nuts_massgiven.hpp:166:28: error: no matching function for call to 'dot_self'
        double H0 = -0.5 * stan::math::dot_self(mminus) + this->_logp;

Same fix as 456629d

src/stan/mcmc/nuts_massgiven.hpp:358:22: error: no matching function for call to 'min'
          prob_sum = stan::math::min(1, exp(newH - H0));

Same fix as 95b4dee


src/stan/gm/function_signatures.hpp
...
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

I don't know how these function_signatures.hpp files are supposed to work since they don't have any #includes

In file included from src/stan/gm/grammars/statement_2_grammar.hpp:14:
src/stan/gm/grammars/statement_grammar.hpp:39:7: error: no template named 'statement_2_grammar'; did you mean 'statement_grammar'?
      statement_2_grammar<Iterator> statement_2_g;

Not sure what is going wrong here because the statement_2_grammar is #included

src/stan/math/matrix/EigenDenseBaseAddons.hpp:1:18: error: use of undeclared identifier 'internal'
typedef typename internal::traits<Derived>::Index size_type; 

src/stan/math/matrix/EigenDenseBaseAddons.hpp:2:18: error: use of undeclared identifier 'internal'
typedef typename internal::traits<Derived>::Scalar value_type;

I think these two are valid, but I don't know if there are any workarounds.

src/stan/math/functions/scaled_add.hpp:13:12: error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'?

git show bd513b2
...
+#include <cstddef>

src/stan/math/functions/dist.hpp:12:12: error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'?

Same fix as bd513b2

src/stan/math/functions/dist.hpp:10:18: error: no member named 'sqrt' in namespace 'std'
      using std::sqrt;
 
git show f51a02c
...
+#include <cmath>

src/stan/math/functions/dot.hpp:13:12: error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'?

Same fix as bd513b2

src/stan/math/functions/sub.hpp:12:12: error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'?

Same fix as bd513b2

src/stan/math/functions/sum.hpp:11:12: error: unknown type name 'size_t'; did you mean '__gnu_cxx::size_t'?

Same fix as bd513b2

Ben

[Ben Goodrich]

On Tuesday, April 9, 2013 6:01:43 PM UTC-4, Bob Carpenter wrote:

Sorry -- I meant spurious "usings", not spurious "includes".
Just grepping, it's hard to see the context of the using statements
and I want to check we don't have any outside of function
scopes.

Getting rid of spurious includes is good, too.
We should run this again and clean up now that Daniel's
refactored so much of the math lib.

Attached is the result of

goodrich@CYBERPOWERPC:/tmp/llvm/build/bin$ export CFLAGS="-I /opt/stan/src -I /opt/stan/lib/eigen_3.1.2/ -I /opt/stan/lib/boost_1.53.0/ -Wall -DBOOST_RESULT_OF_USE_TR1 -DBOOST_NO_DECLTYPE -DBOOST_DISABLE_ASSERTS"

goodrich@CYBERPOWERPC:/tmp/llvm/build/bin$ find /opt/stan/src/stan -name '*.hpp' -print0 | parallel -0 -j7 ./include-what-you-use $CFLAGS {} "2>" {}_iwyu.txt

goodrich@CYBERPOWERPC:/tmp/llvm/build/bin$ cd /opt/stan/src/stan

goodrich@CYBERPOWERPC:/opt/stan/src/stan$ find . -name '*_iwyu.txt' -print0 | xargs -0 tar -rf /tmp/iwyu.tar

goodrich@CYBERPOWERPC:/opt/stan/src/stan$ gzip /tmp/iwyu.tar

Thus, the foo.hpp_iwyu.txt files that say what should and should not be included in foo.hpp can be unzipped relative to $STAN_HOME/src/stan but you might want to instead do

cd /tmp
mkdir -p STAN_TEMP/src/stan
cd STAN_TEMP/src/stan
tar -xvf /path/to/iwyu.tar.gz .

Ben


Ben

[Daniel Lee]

Hi Ben,

Sorry, but can you explain what this output is supposed to tell us? It looks like there are a lot of non-issues in here. For example:

src/stan/math/matrix/typedefs.hpp:9:66: error: no type named 'size_type' in 'Eigen::Matrix<double, -1, -1, 0, -1, -1>'
    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;

git show 5911afb

...
-    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;

+    typedef size_t size_type;

This comes from us extending Eigen's base type with an EigenDenseMatrixPlugin so we can exchange std::vector and Eigen (row-)vectors and use appropriate types for indexing.

So, I guess I'll need a little more help understanding the output for this to be useful. I'm just not sure what is supposed to be done.

Daniel

[Daniel Lee]

Hi Ben,

Regarding iwyu, thanks for running it. There's some good information here, but not all of it's accurate. Can you run it again next week after we've pushed refactored code into master? I'll try to follow as much of its advice as I can.

One thing it does really poorly with are headers that are used by our model header. I guess it can't tell how much is required for a model that could cover the entire language. If there's a way to configure that, the output would probably look a lot more useful.

Thanks again. Can we revisit this after Michael's pushed the refactoring?


Daniel

[Ben Goodrich]

On Sunday, April 14, 2013 11:16:42 PM UTC-4, Daniel Lee wrote:

Hi Ben,

Sorry, but can you explain what this output is supposed to tell us? It looks like there are a lot of non-issues in here. For example:

src/stan/math/matrix/typedefs.hpp:9:66: error: no type named 'size_type' in 'Eigen::Matrix<double, -1, -1, 0, -1, -1>'
    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;

git show 5911afb

...
-    typedef Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic>::size_type size_type;

+    typedef size_t size_type;

This comes from us extending Eigen's base type with an EigenDenseMatrixPlugin so we can exchange std::vector and Eigen (row-)vectors and use appropriate types for indexing.

I wasn't 100% sure about that one. But I presumed that Eigen::MatrixXd::size_type didn't exist because I couldn't find that syntax in the eigen tree for a MatrixXd

goodrich@CYBERPOWERPC:/opt/eigen$ hg grep "::size_type" Eigen/
Eigen/src/StlSupport/StdDeque.h:4751:    typedef typename deque_base::size_type size_type;  \
Eigen/src/StlSupport/StdDeque.h:4751:    typedef typename deque_base::size_type size_type;  \
Eigen/src/StlSupport/StdList.h:4751:    typedef typename list_base::size_type size_type;  \
Eigen/src/StlSupport/StdList.h:4751:    typedef typename list_base::size_type size_type;  \
Eigen/src/StlSupport/StdVector.h:4751:    typedef vector_base::size_type size_type;  \
Eigen/src/StlSupport/StdVector.h:4751:    typedef typename vector_base::size_type size_type;  \
Eigen/NewStdVector:2630:    typedef typename vector_base::size_type size_type;  \
Eigen/StdVector:2389:    typedef typename vector_base::size_type size_type;  \
Eigen/StdVector:2389:    typedef typename vector_base::size_type size_type;  \
Eigen/src/StdVector/StdVector.h:796:  typedef typename unaligned_base::size_type size_type; \

Ben

[Daniel Lee]

Yes, we wrote an EigenDenseMatrixPlugin. See src/stan/math/matrix/EigenDenseBaseAddons.hpp and how we utilize the plugin in the file Eigen.hpp in the same folder.

[Ben Goodrich]

I can do it again anytime, but according to the include-what-you-use principle, something like

goodrich@CYBERPOWERPC:/opt/stan$ cat src/stan.hpp
#ifndef STAN_HPP
#define STAN_HPP

// #include "stan/gm.hpp"
#include "stan/io.hpp"
#include "stan/memory.hpp"
#include "stan/mcmc.hpp"
#include "stan/meta.hpp"
#include "stan/model.hpp"
#include "stan/prob.hpp"
#include "stan/version.hpp"

#endif

should just be deleted because whatever might need any of the above .hpp files should be #including it directly rather than #including <stan.hpp> and pulling in those things indirectly. That said, I don't know if it is possible to fully adhere to the include-what-you-use principle, particularly with the model header. I did a model once before at

https://groups.google.com/d/msg/stan-dev/q977l_VE2Ls/0spLJnlceXIJ

and it suggested a lot of changes, but I don't know if it is possible to make the parser sophisticated enough to do that for an arbitrary model.

Ben

[Ben Goodrich]

On Monday, April 15, 2013 12:56:36 AM UTC-4, Daniel Lee wrote:

Yes, we wrote an EigenDenseMatrixPlugin. See src/stan/math/matrix/EigenDenseBaseAddons.hpp and how we utilize the plugin in the file Eigen.hpp in the same folder.

Ah, I wasn't too sure what to do about that file either (although I think ::Index is always a size_t). But now I don't understand why if src/stan/math/matrix/typedefs.hpp #includes src/stan/math/matrix/Eigen.hpp and the later #defines EIGEN_DENSEBASE_PLUGIN "stan/math/matrix/EigenDenseBaseAddons.hpp" and that typedefs size_type why clang++ said that there was no such thing.

In any event, those were probably among the most minor issues in the list.

Ben


 

[Daniel Lee]

Thanks for pointing me to the original email. It must have slipped by me while I was refactoring the math libs. 

On Monday, April 15, 2013, Ben Goodrich wrote:

I can do it again anytime, but according to the include-what-you-use principle, something like

goodrich@CYBERPOWERPC:/opt/stan$ cat src/stan.hpp
#ifndef STAN_HPP
#define STAN_HPP

// #include "stan/gm.hpp"
#include "stan/io.hpp"
#include "stan/memory.hpp"
#include "stan/mcmc.hpp"
#include "stan/meta.hpp"
#include "stan/model.hpp"
#include "stan/prob.hpp"
#include "stan/version.hpp"

#endif

should just be deleted because whatever might need any of the above .hpp files should be #including it directly rather than #including <stan.hpp> and pulling in those things indirectly. That said, I don't know if it is possible to fully adhere to the include-what-you-use principle, particularly with the model header. I did a model once before at

https://groups.google.com/d/msg/stan-dev/q977l_VE2Ls/0spLJnlceXIJ

and it suggested a lot of changes, but I don't know if it is possible to make the parser sophisticated enough to do that for an arbitrary model.

Ben

Although I agree in principle, I don't think it's that easy to build a minimal include list for arbitrary models that may be generated by users. If you wanted to run it on something that represents the language as a whole, if you can figure out a way to paste together all the Stan models we compile to verify parsing and generation, that would be worthwhile. The files are located under src/test/gm/model_specs/compiled/. 

[Ben Goodrich]

On Monday, April 15, 2013 1:23:17 AM UTC-4, Daniel Lee wrote:

Although I agree in principle, I don't think it's that easy to build a minimal include list for arbitrary models that may be generated by users. If you wanted to run it on something that represents the language as a whole, if you can figure out a way to paste together all the Stan models we compile to verify parsing and generation, that would be worthwhile. The files are located under src/test/gm/model_specs/compiled/. 

Yeah, we could worry about user models later and hopefully iwyu will help with everything else. It is certainly easiest to have a model_header.hpp that indirectly pulls in every function that a user could utilize but it also means a lot of unnecessary stuff is getting pulled in for each particular model. I would think that the only way to square that circle would be to have a giant lookup table that said for each exposed function which .hpp file defines that function.

Ben

[Bob Carpenter]

Indexes in Eigen are ints :-( So as not to
get gripes from the compiler about signed/unsigned
assignments, we need to use the typedefs in polymorphic
code.

- Bob

[Ben Goodrich]

On Monday, April 15, 2013 2:18:56 AM UTC-4, Bob Carpenter wrote:

Indexes in Eigen are ints :-(  So as not to
get gripes from the compiler about signed/unsigned
assignments, we need to use the typedefs in polymorphic
code.

I was pretty sure that got changed for Eigen 3.0
http://listengine.tuxfamily.org/lists.tuxfamily.org/eigen/2010/05/msg00161.html

It can be set:
http://eigen.tuxfamily.org/dox-devel/classEigen_1_1DenseBase.html#a4d4873e91be950c079f067fa97fd5c40

which refers to
http://eigen.tuxfamily.org/dox-devel/TopicPreprocessorDirectives.html

which says:
EIGEN_DEFAULT_DENSE_INDEX_TYPE - the type for column and row indices in matrices, vectors and array (DenseBase::Index). Set to std::ptrdiff_t by default.

So, I was wrong about remembering it being a size_t, and it is still signed. My commit should be reverted, but I'm still not sure why it was failing before.

Ben

[Ben Goodrich]

Another iteration of this, after 542d914d3a056bf3a866ad7167f98083cd7018d1. I am also including the result of

goodrich@CYBERPOWERPC:/opt/stan/src/stan$ find . -name '*_iwyu.txt' -print0 | xargs -0 wc -l | sort -g -r > /tmp/iwyu_sorted.txt

which sorts the foo.hpp_iwyu.txt files by descending order of line count, which loosely corresponds to how much iwyu thinks should be changed but can be inflated by warnings and whatnot.

Ben

[Bob Carpenter]

Thanks --- Daniel and I can look into this next
week.

- Bob