| Message ID | 20200610194945.950953-1-fbl@sysclose.org |
|---|---|
| State | Accepted |
| Commit | 9ed9df77a3097146addd9cc2f53bffebd71c1343 |
| Headers | show |
| Series | [ovs-dev] ctags: Include new annotations to ctags ignore list. | expand |
Hi, It would be nice to have this applied to branch-2.13 as well. fbl On Wed, Jun 10, 2020 at 04:49:45PM -0300, Flavio Leitner wrote: > The annotation OVS_NO_THREAD_SAFETY_ANALYSIS and OVS_LOCKABLE are > not part of the list, so ctags can't find functions using them. > > The annotation list comes from a regex and to include more items > make the regex more difficult to read and maintain. Convert to a > static list because it isn't supposed to change much and there > is no standard names. > > Also add a comment to remind to keep the list up-to-date. > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > --- > Makefile.am | 2 +- > acinclude.m4 | 6 +++--- > include/openvswitch/compiler.h | 2 ++ > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/Makefile.am b/Makefile.am > index b279303d1..27ef9e4b4 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -46,7 +46,7 @@ AM_CPPFLAGS += -DNDEBUG > AM_CFLAGS += -fomit-frame-pointer > endif > > -AM_CTAGSFLAGS = $(OVS_CTAGS_IDENTIFIERS_LIST) > +AM_CTAGSFLAGS = -I "$(OVS_CTAGS_IDENTIFIERS_LIST)" > > if WIN32 > psep=";" > diff --git a/acinclude.m4 b/acinclude.m4 > index 8847b8145..054ec2e3c 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -1332,11 +1332,11 @@ AC_DEFUN([OVS_ENABLE_SPARSE], > > dnl OVS_CTAGS_IDENTIFIERS > dnl > -dnl ctags ignores symbols with extras identifiers. This builds a list of > -dnl specially handled identifiers to be ignored. > +dnl ctags ignores symbols with extras identifiers. This is a list of > +dnl specially handled identifiers to be ignored. [ctags(1) -I <list>]. > AC_DEFUN([OVS_CTAGS_IDENTIFIERS], > AC_SUBST([OVS_CTAGS_IDENTIFIERS_LIST], > - [`printf %s '-I "'; sed -n 's/^#define \(OVS_[A-Z_]\+\)(\.\.\.)$/\1+/p' ${srcdir}/include/openvswitch/compiler.h | tr \\\n ' ' ; printf '"'`] )) > + ["OVS_LOCKABLE OVS_NO_THREAD_SAFETY_ANALYSIS OVS_REQ_RDLOCK+ OVS_ACQ_RDLOCK+ OVS_REQ_WRLOCK+ OVS_ACQ_WRLOCK+ OVS_REQUIRES+ OVS_ACQUIRES+ OVS_TRY_WRLOCK+ OVS_TRY_RDLOCK+ OVS_TRY_LOCK+ OVS_GUARDED_BY+ OVS_EXCLUDED+ OVS_RELEASES+ OVS_ACQ_BEFORE+ OVS_ACQ_AFTER+"])) > > dnl OVS_PTHREAD_SET_NAME > dnl > diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h > index 5289a70f6..cf009f826 100644 > --- a/include/openvswitch/compiler.h > +++ b/include/openvswitch/compiler.h > @@ -113,6 +113,8 @@ > * OVS_REQUIRES OVS_REQ_RDLOCK OVS_REQ_WRLOCK > * OVS_EXCLUDED OVS_EXCLUDED OVS_EXCLUDED > */ > + > +/* Please keep OVS_CTAGS_IDENTIFIERS up-to-date in acinclude.m4. */ > #define OVS_LOCKABLE __attribute__((lockable)) > #define OVS_REQ_RDLOCK(...) __attribute__((shared_locks_required(__VA_ARGS__))) > #define OVS_ACQ_RDLOCK(...) __attribute__((shared_lock_function(__VA_ARGS__))) > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Jun 10, 2020 at 04:49:45PM -0300, Flavio Leitner wrote: > The annotation OVS_NO_THREAD_SAFETY_ANALYSIS and OVS_LOCKABLE are > not part of the list, so ctags can't find functions using them. > > The annotation list comes from a regex and to include more items > make the regex more difficult to read and maintain. Convert to a > static list because it isn't supposed to change much and there > is no standard names. > > Also add a comment to remind to keep the list up-to-date. > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> Hi Flavio, Instead of a static list, how about adding sed -n -e 's/^#define \(OVS_[A-Z_]\+\)(\.\.\.)$/\1+/p' \ -e 's/^#define \(OVS_[A-Z_]\+\)$/\1+/p' include/openvswitch/compiler.h So with the 2nd sed command, it creates OVS_NO_RETURN+ OVS_UNUSED+ OVS_WARN_UNUSED_RESULT+ OVS_LOCKABLE+ OVS_GUARDED+ OVS_NO_THREAD_SAFETY_ANALYSIS+ OVS_PACKED_ENUM+ Which covers the OVS_NO_THREAD_SAFETY_ANALYSIS+ and others. I'm also ok keeping it static, if so, should we add these above? Thanks William
On Sat, Jun 27, 2020 at 05:11:15PM -0700, William Tu wrote: > On Wed, Jun 10, 2020 at 04:49:45PM -0300, Flavio Leitner wrote: > > The annotation OVS_NO_THREAD_SAFETY_ANALYSIS and OVS_LOCKABLE are > > not part of the list, so ctags can't find functions using them. > > > > The annotation list comes from a regex and to include more items > > make the regex more difficult to read and maintain. Convert to a > > static list because it isn't supposed to change much and there > > is no standard names. > > > > Also add a comment to remind to keep the list up-to-date. > > > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > > Hi Flavio, > > Instead of a static list, how about adding > sed -n -e 's/^#define \(OVS_[A-Z_]\+\)(\.\.\.)$/\1+/p' \ > -e 's/^#define \(OVS_[A-Z_]\+\)$/\1+/p' include/openvswitch/compiler.h > > So with the 2nd sed command, it creates > OVS_NO_RETURN+ > OVS_UNUSED+ > OVS_WARN_UNUSED_RESULT+ > OVS_LOCKABLE+ > OVS_GUARDED+ > OVS_NO_THREAD_SAFETY_ANALYSIS+ > OVS_PACKED_ENUM+ > > Which covers the OVS_NO_THREAD_SAFETY_ANALYSIS+ and others. The '+' is required only when parenthesis could be used, so it wouldn't apply to OVS_NO_RETURN, for instance. I don't know the side effects. Another thing is that the '-I' parameter is useful when an identifier causes syntactic confusion. I can navigate to symbols using OVS_UNUSED and I don't see any problems. Same for OVS_NO_RETURN. Funny thing is that I had issues with OVS_LOCKABLE but not anymore. I did upgrade my fedora in between, so not sure. Anyways, I still have issues navigating to dpdk_do_tx_copy() because of OVS_NO_THREAD_SAFETY_ANALYSIS. It works if the identifier is included in the list. > I'm also ok keeping it static, if so, should we add these above? I understand the goal of fixing one time and leave it automatically, but ctags man-page is clear about using the parameter when an identifier causes problems. I don't know what else it does, so I took the safe approach. What do you think? Thanks,
On Tue, Jun 30, 2020 at 8:33 AM Flavio Leitner <fbl@sysclose.org> wrote: > > On Sat, Jun 27, 2020 at 05:11:15PM -0700, William Tu wrote: > > On Wed, Jun 10, 2020 at 04:49:45PM -0300, Flavio Leitner wrote: > > > The annotation OVS_NO_THREAD_SAFETY_ANALYSIS and OVS_LOCKABLE are > > > not part of the list, so ctags can't find functions using them. > > > > > > The annotation list comes from a regex and to include more items > > > make the regex more difficult to read and maintain. Convert to a > > > static list because it isn't supposed to change much and there > > > is no standard names. > > > > > > Also add a comment to remind to keep the list up-to-date. > > > > > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > > > > Hi Flavio, > > > > Instead of a static list, how about adding > > sed -n -e 's/^#define \(OVS_[A-Z_]\+\)(\.\.\.)$/\1+/p' \ > > -e 's/^#define \(OVS_[A-Z_]\+\)$/\1+/p' include/openvswitch/compiler.h > > > > So with the 2nd sed command, it creates > > OVS_NO_RETURN+ > > OVS_UNUSED+ > > OVS_WARN_UNUSED_RESULT+ > > OVS_LOCKABLE+ > > OVS_GUARDED+ > > OVS_NO_THREAD_SAFETY_ANALYSIS+ > > OVS_PACKED_ENUM+ > > > > Which covers the OVS_NO_THREAD_SAFETY_ANALYSIS+ and others. > > The '+' is required only when parenthesis could be used, so > it wouldn't apply to OVS_NO_RETURN, for instance. I don't know > the side effects. I see, thanks! > > Another thing is that the '-I' parameter is useful when an > identifier causes syntactic confusion. I can navigate to symbols > using OVS_UNUSED and I don't see any problems. Same for OVS_NO_RETURN. > > Funny thing is that I had issues with OVS_LOCKABLE but not anymore. > I did upgrade my fedora in between, so not sure. > > Anyways, I still have issues navigating to dpdk_do_tx_copy() because > of OVS_NO_THREAD_SAFETY_ANALYSIS. It works if the identifier is > included in the list. > > > I'm also ok keeping it static, if so, should we add these above? > > I understand the goal of fixing one time and leave it automatically, > but ctags man-page is clear about using the parameter when an > identifier causes problems. I don't know what else it does, so I > took the safe approach. > > What do you think? Thanks for explaining this. In this case, I think keeping it static makes sense. William
On Wed, Jun 10, 2020 at 08:49:08PM -0300, Flavio Leitner wrote: > > Hi, > > It would be nice to have this applied to branch-2.13 as well. > fbl I applied to master and 2.13. Thanks! William
diff --git a/Makefile.am b/Makefile.am index b279303d1..27ef9e4b4 100644 --- a/Makefile.am +++ b/Makefile.am @@ -46,7 +46,7 @@ AM_CPPFLAGS += -DNDEBUG AM_CFLAGS += -fomit-frame-pointer endif -AM_CTAGSFLAGS = $(OVS_CTAGS_IDENTIFIERS_LIST) +AM_CTAGSFLAGS = -I "$(OVS_CTAGS_IDENTIFIERS_LIST)" if WIN32 psep=";" diff --git a/acinclude.m4 b/acinclude.m4 index 8847b8145..054ec2e3c 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -1332,11 +1332,11 @@ AC_DEFUN([OVS_ENABLE_SPARSE], dnl OVS_CTAGS_IDENTIFIERS dnl -dnl ctags ignores symbols with extras identifiers. This builds a list of -dnl specially handled identifiers to be ignored. +dnl ctags ignores symbols with extras identifiers. This is a list of +dnl specially handled identifiers to be ignored. [ctags(1) -I <list>]. AC_DEFUN([OVS_CTAGS_IDENTIFIERS], AC_SUBST([OVS_CTAGS_IDENTIFIERS_LIST], - [`printf %s '-I "'; sed -n 's/^#define \(OVS_[A-Z_]\+\)(\.\.\.)$/\1+/p' ${srcdir}/include/openvswitch/compiler.h | tr \\\n ' ' ; printf '"'`] )) + ["OVS_LOCKABLE OVS_NO_THREAD_SAFETY_ANALYSIS OVS_REQ_RDLOCK+ OVS_ACQ_RDLOCK+ OVS_REQ_WRLOCK+ OVS_ACQ_WRLOCK+ OVS_REQUIRES+ OVS_ACQUIRES+ OVS_TRY_WRLOCK+ OVS_TRY_RDLOCK+ OVS_TRY_LOCK+ OVS_GUARDED_BY+ OVS_EXCLUDED+ OVS_RELEASES+ OVS_ACQ_BEFORE+ OVS_ACQ_AFTER+"])) dnl OVS_PTHREAD_SET_NAME dnl diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h index 5289a70f6..cf009f826 100644 --- a/include/openvswitch/compiler.h +++ b/include/openvswitch/compiler.h @@ -113,6 +113,8 @@ * OVS_REQUIRES OVS_REQ_RDLOCK OVS_REQ_WRLOCK * OVS_EXCLUDED OVS_EXCLUDED OVS_EXCLUDED */ + +/* Please keep OVS_CTAGS_IDENTIFIERS up-to-date in acinclude.m4. */ #define OVS_LOCKABLE __attribute__((lockable)) #define OVS_REQ_RDLOCK(...) __attribute__((shared_locks_required(__VA_ARGS__))) #define OVS_ACQ_RDLOCK(...) __attribute__((shared_lock_function(__VA_ARGS__)))
The annotation OVS_NO_THREAD_SAFETY_ANALYSIS and OVS_LOCKABLE are not part of the list, so ctags can't find functions using them. The annotation list comes from a regex and to include more items make the regex more difficult to read and maintain. Convert to a static list because it isn't supposed to change much and there is no standard names. Also add a comment to remind to keep the list up-to-date. Signed-off-by: Flavio Leitner <fbl@sysclose.org> --- Makefile.am | 2 +- acinclude.m4 | 6 +++--- include/openvswitch/compiler.h | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-)