Re: [dpdk-dev] [PATCH v3] ip_pipeline: add script file for pipeline to core mappings

["Mcnamara, John" <john.mcnamara@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jasvinder Singh
> Sent: Tuesday, May 31, 2016 9:58 AM
> To: dev@dpdk.org
> Cc: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Rao, GuruprasadX
> <guruprasadx.rao@intel.com>
> Subject: [dpdk-dev] [PATCH v3] ip_pipeline: add script file for pipeline
> to core mappings
> 
> From: Guruprasad Mukundarao <guruprasadx.rao@intel.com>
> 

Hi Guruprasad, 

Could you make the Python code PEP8 (Style Guide for Python Code) compliant: 

    https://www.python.org/dev/peps/pep-0008/

You can use the pep8 tool to test code for compliance.


Also, the code is Python3 specific, you should also make it work with Python2.
This mainly requires adding the following at the start of the program to get
the Python3 print(end='') syntax:

    from __future__ import print_function

Also, remove the unnecessary Unicode characters in a couple of the comments.


> +def len2mask(length):
> +    if (length == 0):
> +        return 0
> +
> +    if (length > 64):
> +        sys.exit('error: len2mask - lenght %i > 64. exiting' %length)
> +
> +    return (0xFFFFFFFFFFFFFFFF >> (64 - length))


The following is another way to do it:

    return int('1' * length, 2)


> +
> +def bitstring_write(n, n_bits):
> +    tmpstr = ""
> +    if (n_bits > 64):
> +        return
> +
> +    i = n_bits - 1
> +    while (i >= 0):
> +        cond = (n & (1 << i))
> +        if (cond):
> +            print('1', end='')
> +            tmpstr += '1'
> +        else:
> +            print('0', end='')
> +            tmpstr += '0'
> +        i -= 1
> +    #end while
> +    return tmpstr
> +#end function

Do you really need to print the string *and* return it. Maybe better to just return it and print it out then.

The following would also work as an alternative:

    return '{{0:0{}b}}'.format(n_bits).format(n)


> +    #end while
> +    return tmpstr
> +#end function

Leave out all of these #end comments.


> Constants = namedtuple('Constants', ['MAX_CORES', 'MAX_PIPELINES'])
> +constants = Constants(16, 64)

Move these to the start of the program.


> +#----------------------------------------------------------------------
> +class Cores0:

Leave out these long #----- visual separator and add comments, or docstrings,
explaining the classes instead.

> +class Context0:
> +    def __init__(self):
> +        self.cores = [Cores0() for i in range(0, constants.MAX_CORES)]
> +        self.n_cores = 0
> +        self.n_pipelines = 0
> +        self.n_pipelines0 = 0
> +        self.pos = 0
> +        self.file_comment = ""
> +        self.c1 = None
> +        self.c2 = None

Better to use more descriptive member names than c1 and c2.


> +#-------------------------------------------------------------
> +class Context1:
> +    #class attribute

This type of comment isn't required.



> +        #write out the comments
> +        outputFile.write("; =============== Pipeline-to-Core Mapping
> ================\n")
> +        outputFile.write("; Generated from file
> {}\n".format(self._fileTrace.in_file_namepath))
> +        outputFile.write("; Input pipelines = {}\n; Input cores = {}\n" \
> +                .format(fileTrace.arr_pipelines,
> + fileTrace.in_physical_cores))
> +
> +        strTruncated = ("", "(Truncated)")
> [self._fileTrace.ncores_truncated]
> +        outputFile.write("; N_PIPELINES = {} N_CORES = {} {} hyper_thread
> = {} \n"\
> +                .format(self._fileTrace.n_pipelines,
> + self._fileTrace.n_cores, strTruncated, self._fileTrace.hyper_thread))
> +
> +        outputFile.write("; {}\n".format(self.stage0_file_comment))
> #stage0 comment
> +        outputFile.write("; {}\n".format(self.stage1_file_comment))
> #stage1 comment
> +        #debugging
> +        #outputFile.write("; <<<<printing stage1
> arr_pipelines2cores>>>>")
> +        #outputFile.write("; stage1_arr_pipelines2cores =
> {}".format(self.arr_pipelines2cores))
> +        outputFile.write(";
> ========================================================\n")


This might be better as a multi-line string with a .format() at the end.
It would look more like the comment the code is producing.



> +#----------------------------------------------------------------------
> +------- # python trick - so that our Python files can act as either
> +reusable modules, # or as standalone programs if __name__ ==
> +"__main__":

No need for this comment but other comments throughout the code would be
useful.

Also, there is debug/commented out code in several places that should be
removed.

John